-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Adding resilience_mode field support to Cloud Composer #8058
Adding resilience_mode field support to Cloud Composer #8058
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Oops! It looks like no changelog entry is attached to this PR. Please include a release note block in the PR body, as described in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md:
|
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @roaks3, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
Oops! It looks like no changelog entry is attached to this PR. Please include a release note block in the PR body, as described in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md:
|
mmv1/third_party/terraform/resources/resource_composer_environment.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb
Show resolved
Hide resolved
mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb
Show resolved
Hide resolved
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. Terraform GA: Diff ( 5 files changed, 267 insertions(+), 175 deletions(-)) Missing test reportYour PR includes resource fields which are not covered by any test. Resource: Please add acceptance tests which include these fields. |
Tests analyticsTotal tests: Errors occurred during REPLAYING mode. Please fix them to complete your PR |
It looks like there's a build error:
|
29d1154
to
4799cd1
Compare
4799cd1
to
3be2396
Compare
Tests analyticsTotal tests: Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeNetworkEndpoints_networkEndpointsBasic|TestAccComputeFirewallPolicyRule_multipleRules|TestAccComposerEnvironment_ComposerV2HighResilience|TestAccComposerEnvironment_ComposerV2 |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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.
LGTM and tests pass. I had a few small questions/suggestions, but nothing major.
mmv1/third_party/terraform/resources/resource_composer_environment.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/resources/resource_composer_environment.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/tests/resource_composer_environment_test.go.erb
Outdated
Show resolved
Hide resolved
resilience_mode = "HIGH_RESILIENCE" | ||
private_environment_config { | ||
enable_private_endpoint = true | ||
cloud_composer_network_ipv4_cidr_block = "10.3.192.0/24" |
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.
I'm a little out of my depth so want to make sure: is there any risk of these cidr ranges conflicting with other resources in our various tests environments? (for example, if this test runs at the same time as the test above, testAccComposerEnvironment_composerV2)
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.
I'm not sure if there is a risk. Nonetheless, these IP ranges have been duplicated several times already in these tests. I expect that duplicating them once more shouldn't change much.
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.
Oh alright, I hadn't noticed that, but agreed it shouldn't be an issue then.
mmv1/third_party/terraform/website/docs/r/composer_environment.html.markdown
Outdated
Show resolved
Hide resolved
Thanks! I addressed the comments. PTAL. |
This is ready to merge, but we'll need to resolve the conflicts first, and then just double check that tests still pass. |
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. Terraform GA: Diff ( 3 files changed, 127 insertions(+)) |
Tests analyticsTotal tests: Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeNetworkEndpoints_networkEndpointsBasic|TestAccComposerEnvironment_ComposerV2HighResilience|TestAccComputeFirewallPolicyRule_multipleRules |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
VCR-test is marked as failed but when I click Details, it shows as successful in Cloud Build. What can be the issue here? |
@roaks3 I've tried running |
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. Terraform GA: Diff ( 3 files changed, 127 insertions(+)) |
This is the error from the failed test. Per https://cloud.google.com/build/docs/iam-roles-permissions, it looks like this role should be there by default when the service is enabled. It's possible some other test or PR is affecting that permission, but I don't think it is a concern of this test. In other words, we should be able to safely assume that role exists:
|
Tests analyticsTotal tests: Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeNetworkEndpoints_networkEndpointsBasic|TestAccComposerEnvironment_ComposerV2HighResilience|TestAccComputeFirewallPolicyRule_multipleRules |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Merging because the tests passed this time, and the missing role that caused the failure should not be an issue under normal circumstances. |
…atform#8058) * Adding High Resilience Fields Support To Terraform * Bumping composer api version to 0.125.0 * Adding Test for composerV2HighResilience * Fixing function name * retrigger checks * retrigger checks * Replacing go.sum with the fresh version * Replacing go.mod.erb with the fresh version * Removing high resilience test and adding field to composerv2 test * Fixing Indentation * Remove HighResilience TEST * Removing HR test * Fixing Removing ComposerV2 test * Fixing go mod and go sum * Added Test for HighResilience * Fixed Indentation * Revert go.mod.erb and go.sum changes * Apply review comments * retrigger checks --------- Co-authored-by: Khaled Hassan <hkhaled@google.com> Co-authored-by: Ryan Oaks <ryanoaks@google.com> Co-authored-by: Przemek Wiech <pwiech@google.com>
…atform#8058) * Adding High Resilience Fields Support To Terraform * Bumping composer api version to 0.125.0 * Adding Test for composerV2HighResilience * Fixing function name * retrigger checks * retrigger checks * Replacing go.sum with the fresh version * Replacing go.mod.erb with the fresh version * Removing high resilience test and adding field to composerv2 test * Fixing Indentation * Remove HighResilience TEST * Removing HR test * Fixing Removing ComposerV2 test * Fixing go mod and go sum * Added Test for HighResilience * Fixed Indentation * Revert go.mod.erb and go.sum changes * Apply review comments * retrigger checks --------- Co-authored-by: Khaled Hassan <hkhaled@google.com> Co-authored-by: Ryan Oaks <ryanoaks@google.com> Co-authored-by: Przemek Wiech <pwiech@google.com>
…atform#8058) * Adding High Resilience Fields Support To Terraform * Bumping composer api version to 0.125.0 * Adding Test for composerV2HighResilience * Fixing function name * retrigger checks * retrigger checks * Replacing go.sum with the fresh version * Replacing go.mod.erb with the fresh version * Removing high resilience test and adding field to composerv2 test * Fixing Indentation * Remove HighResilience TEST * Removing HR test * Fixing Removing ComposerV2 test * Fixing go mod and go sum * Added Test for HighResilience * Fixed Indentation * Revert go.mod.erb and go.sum changes * Apply review comments * retrigger checks --------- Co-authored-by: Khaled Hassan <hkhaled@google.com> Co-authored-by: Ryan Oaks <ryanoaks@google.com> Co-authored-by: Przemek Wiech <pwiech@google.com>
Add Cloud Composer Support for high resilience field
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
in the generated providers to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)
Fixes hashicorp/terraform-provider-google/issues/14751