-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Only set enabled bool if evaluation mode is unspecified #9629
Only set enabled bool if evaluation mode is unspecified #9629
Conversation
Hello! I am a robot. It looks like you are a: @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. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Unrelated? I don't see any of the affected tests failing |
mmv1/third_party/terraform/services/container/resource_container_cluster.go.erb
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
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 ( 1 file changed, 12 insertions(+), 27 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccContainerCluster_withBinaryAuthorizationEnabledBool |
Rerun these tests in REPLAYING mode to catch issues
|
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 ( 1 file changed, 13 insertions(+), 28 deletions(-)) |
Tests analyticsTotal tests:
|
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.
Looking over this more closely, I'm not sure I fully understand what the "bad case" that we're trying to prevent is. Left some comments below. Let me know what you think!
Additionally, please update https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/website/docs/guides/version_5_upgrade.html.markdown to specify the correct update path for people who had been using enable_binary_authorization in 4.X.
mmv1/third_party/terraform/services/container/resource_container_cluster.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/container/resource_container_cluster.go.erb
Outdated
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 ( 2 files changed, 9 insertions(+), 21 deletions(-)) |
Tests analyticsTotal tests:
|
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.
minor docs tweak
mmv1/third_party/terraform/website/docs/guides/version_5_upgrade.html.markdown
Outdated
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 ( 2 files changed, 9 insertions(+), 21 deletions(-)) |
Tests analyticsTotal tests:
|
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. FWIW I am not able to reproduce the original case that spawned this ticket - the update seems to happen as expected on the main branch (1 single update request) and completes very quickly.
I'm using a different version of Terraform so it's possible that the behavior was changed over time.
All that said, even if the upgrade took two updates, that isn't the main issue. The main issue is correcting the upgrade docs, which this PR does. Eliminating the stray remaining reference to enable_binary_authorization
is also good whether or not it's currently causing problems.
…latform#9629) * Only set enabled bool if evaluation mode is unspecified * Only trigger one cluster update event if both both legacy and struct blocks have diffs. * remove inadvertant modifications * Pass legacy bool to expandBinaryAuthorization fn * move fn for diff readability * reformat * reformat * Only get enable_binary_authorization if its been changed * remove commas * fix logic * Remove enable_binary_authorization condition * Add upgrade path docs * Add comma * Update mmv1/third_party/terraform/website/docs/guides/version_5_upgrade.html.markdown --------- Co-authored-by: Stephen Lewis (Burrows) <[email protected]>
…latform#9629) * Only set enabled bool if evaluation mode is unspecified * Only trigger one cluster update event if both both legacy and struct blocks have diffs. * remove inadvertant modifications * Pass legacy bool to expandBinaryAuthorization fn * move fn for diff readability * reformat * reformat * Only get enable_binary_authorization if its been changed * remove commas * fix logic * Remove enable_binary_authorization condition * Add upgrade path docs * Add comma * Update mmv1/third_party/terraform/website/docs/guides/version_5_upgrade.html.markdown --------- Co-authored-by: Stephen Lewis (Burrows) <[email protected]>
…latform#9629) * Only set enabled bool if evaluation mode is unspecified * Only trigger one cluster update event if both both legacy and struct blocks have diffs. * remove inadvertant modifications * Pass legacy bool to expandBinaryAuthorization fn * move fn for diff readability * reformat * reformat * Only get enable_binary_authorization if its been changed * remove commas * fix logic * Remove enable_binary_authorization condition * Add upgrade path docs * Add comma * Update mmv1/third_party/terraform/website/docs/guides/version_5_upgrade.html.markdown --------- Co-authored-by: Stephen Lewis (Burrows) <[email protected]>
Remove
enable_binary_authorization
conditional, this was missed in #8784.Fixes hashicorp/terraform-provider-google#16759