-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes issue #18911 for Eventarc test flakiness due to IAM races #13200
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @tommyreddad, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
This pull request addresses issue #18911, which involves flakiness in Eventarc tests due to IAM races. The solution implemented here is to bootstrap IAM permissions for CMEK across the entire project, rather than relying on dynamic IAM membership grants. This approach aims to provide more consistent and reliable test execution by ensuring the necessary permissions are in place from the start.
Highlights
- IAM Permissions Bootstrapping: The core change involves bootstrapping IAM permissions for the service account
service-{project_number}@gcp-sa-eventarc.iam.gserviceaccount.com
with theroles/cloudkms.cryptoKeyEncrypterDecrypter
role at the project level. This is done to avoid IAM races during tests. - Resource Configuration Updates: The configuration for Eventarc resources, specifically
google_eventarc_channel_config
andgoogle_eventarc_message_bus
, has been updated to remove dependencies ongoogle_kms_crypto_key_iam_member
resources. The IAM permissions are now handled through bootstrapping. - Test Code Modifications: Test files have been modified to remove the creation of
google_kms_crypto_key_iam_member
resources and instead rely on the bootstrapped IAM permissions. This includes changes toresource_eventarc_google_channel_config_test.go
andresource_eventarc_message_bus_test.go
.
Changelog
Click here to see the changelog
- mmv1/products/eventarc/Trigger.yaml
- Added
bootstrap_iam
configuration to grantroles/cloudkms.cryptoKeyEncrypterDecrypter
to the Eventarc service account on line 59.
- Added
- mmv1/templates/terraform/examples/eventarc_trigger_with_channel_cmek.tf.tmpl
- Removed the
google_kms_crypto_key_iam_member
resource on lines 1-5, as IAM permissions are now bootstrapped.
- Removed the
- mmv1/third_party/terraform/services/eventarc/resource_eventarc_google_channel_config_test.go
- Removed the creation of
google_kms_crypto_key_iam_member
resources on lines 41-42 and 63-73. - Added
acctest.BootstrapIamMembers
to bootstrap IAM permissions for the Eventarc service account on line 42. - Removed
key1
andkey2
variables from the context map on lines 41-42 and 90-91. - Removed
depends_on
from thegoogle_eventarc_google_channel_config
resource on lines 78 and 129.
- Removed the creation of
- mmv1/third_party/terraform/services/eventarc/resource_eventarc_message_bus_test.go
- Removed the creation of
google_kms_crypto_key_iam_member
resources on lines 107-111, 184-188 and 232-236. - Added
acctest.BootstrapIamMembers
to bootstrap IAM permissions for the Eventarc service account on line 42 and 91. - Removed
depends_on
from thegoogle_eventarc_message_bus
resource on lines 120, 198 and 225.
- Removed the creation of
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/
folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Did you know?
The Eventarc service enables you to build event-driven architectures, which are a modern approach to building scalable and loosely coupled applications.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request aims to fix the Eventarc test flakiness issue by bootstrapping IAM permissions for CMEK across the entire project. The changes involve modifying the Trigger.yaml file, removing the google_kms_crypto_key_iam_member resource from the Terraform template, and updating the test files to use the new acctest.BootstrapIamMembers function. Overall, the changes seem reasonable and should address the issue.
Summary of Findings
- IAM Permissions: The pull request bootstraps IAM permissions for CMEK across the entire project, rather than depending on dynamic IAM membership grants, which fixes the Eventarc test flakiness issue.
- Test Configuration: The test configurations have been updated to reflect the new IAM bootstrapping approach.
Assessment
The pull request addresses the Eventarc test flakiness issue by bootstrapping IAM permissions for CMEK across the entire project. The changes look good and should resolve the issue. I've provided some minor feedback for consideration. Please have others review and approve this code before merging.
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @melinath, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
|
Tests analyticsTotal tests: 13 Click here to see the affected service packages
🟢 All tests passed! View the build log |
@melinath This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
Fixes hashicorp/terraform-provider-google#18911
Bootstraps IAM Permissions for CMEK across the entire project, rather than depending on dynamic IAM membership grants.
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.