-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Fix Adding Unexpected Conditions #9547
Fix Adding Unexpected Conditions #9547
Conversation
* Adds no_age field
Hello! I am a robot. It looks like you are a: @roaks3, 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. Terraform GA: Diff ( 3 files changed, 157 insertions(+), 17 deletions(-)) |
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 testsTestAccDataSourceGoogleServiceAccountAccessToken_basic|TestAccDataSourceGoogleServiceAccountIdToken_impersonation|TestAccStorageBucket_lifecycleRuleStateArchived|TestAccStorageBucket_lifecycleRulesNoAge |
Rerun these tests in REPLAYING mode to catch issues
Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer.
|
@roaks3 I don't know why this test failed. When i tried running it locally, It passed. It's unrelated to my PR. |
Adds relevant comments.
Hi there, I'm the Modular magician. I've detected the following information about your changes: Breaking Change Detection FailedThe breaking change detector crashed during execution. This is usually due to the downstream provider(s) failing to compile. Please investigate or follow up with your reviewer. Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 3 files changed, 175 insertions(+), 31 deletions(-)) |
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, 161 insertions(+), 17 deletions(-)) |
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 testsTestAccGKEHub2Fleet_gkehubFleetBasicExample_update|TestAccLoggingProjectSink_updatePreservesCustomWriter|TestAccDataSourceGoogleServiceAccountIdToken_impersonation |
Rerun these tests in REPLAYING mode to catch issues
Tests failed due to non-determinism or randomness when the VCR replayed the response after the HTTP request was made. Please fix these to complete your PR. If you believe these test failures to be incorrect or unrelated to your change, or if you have any questions, please raise the concern with your reviewer.
|
@roaks3 Please review this PR.I don't know why this tests are failing. |
@roaks3 Please Review this 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.
I have some initial spacing nits and questions, but overall the logic here seems good to me, and the tests are great.
The behavior here is something I'm a little unsure about, and the solution doesn't appear to match an existing pattern (although, if you've found a pre-existing pattern could you please share?). It seems that the resource does not behave intuitively at the moment (in that it sends an explicit age of 0
even when not set), and with this change that would continue to be true, but with a workaround where the user can add the no-age
field. I'd like to discuss with my team to make sure there isn't a better way of handling this where the initial problem is fixed to behave as the user would expect.
mmv1/third_party/terraform/services/storage/resource_storage_bucket.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/storage/resource_storage_bucket.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/storage/resource_storage_bucket.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/storage/resource_storage_bucket.go.erb
Show resolved
Hide resolved
mmv1/third_party/terraform/services/storage/resource_storage_bucket.go.erb
Outdated
Show resolved
Hide resolved
transformed.ForceSendFields = append(transformed.ForceSendFields, "Age") | ||
// Setting high precedence of no_age over age when both used together. | ||
// Only sets age value to transformed when no_age has false value | ||
if v, ok := condition["no_age"]; ok && !(v.(bool)) { |
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 wondering about the case where no-age
is not set. It appears to be covered properly in your tests, but I was expecting to see the age
being set even if ok
is false.
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.
no_age
attribute behavior is similar to age
attribute behavior. It is always getting false value because nil value for boolean is false and this attribute is also inside of TypeSet
. Removal of ok
from if condition won't cause issue now but if in future, this issue of getting value every time even when not set, gets properly handle by terraform team then provider starts breaking because of this. We are using v.bool value as condition and suddenly we are not getting no_age from condition
which will cause more problems. Your suggestion are welcome.
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 having a little bit of trouble following on this point. Do you happen to have a link to the Terraform bug you're referencing?
It seems to me you're talking about the treatment of zero-values (ie. an unset no_age
evaluates to false). Is that correct? If so, then we rely on that behavior heavily in the provider, and I wouldn't worry about that changing (it might in the future, if we switch to the plugin framework, but it would be a large change across the entire provider). I believe the case where no_age
is not set will behave properly either way here, but accounting for the !ok
case would help make sure we are covering all branches, and is hypothetically more resilient to things like fields moving.
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.
Okay! In that case i will modify condition like this !ok || !(v.(bool))
which check that no_age
is not found or if it is found then it has false value.
mmv1/third_party/terraform/services/storage/resource_storage_bucket_test.go.erb
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/services/storage/resource_storage_bucket_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 ( 3 files changed, 600 insertions(+), 453 deletions(-)) |
aba1526
to
1347867
Compare
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, 161 insertions(+), 17 deletions(-)) |
@kautikdk oh thanks, that sounds reasonable regarding the approach, thanks for clarifying where this solution came from. Regarding whitespace, it is a bit weird because we have Go code wrapped in ruby templates a lot of the time. And on top of that, some Terraform HCL is written within Go for the test templates. So basically, editors have trouble automatically spacing things properly, and I've sometimes found that I need to manually force sections to be spaces or tabs (depending on the context). |
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 testsTestAccDataprocClusterIamPolicy|TestAccGKEHub2Fleet_gkehubFleetBasicExample_update|TestAccLoggingProjectSink_updatePreservesCustomWriter |
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 ( 3 files changed, 159 insertions(+), 11 deletions(-)) |
d4d079b
to
85029ca
Compare
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, 159 insertions(+), 11 deletions(-)) |
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 testsTestAccDataprocClusterIamPolicy|TestAccGKEHub2Fleet_gkehubFleetBasicExample_update|TestAccHealthcareDatasetIamPolicy |
Rerun these tests in REPLAYING mode to catch issues
|
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 LGTM, will just wait on tests one last time before merge
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, 159 insertions(+), 11 deletions(-)) |
Tests analyticsTotal tests:
|
Note: tweaked the release note |
* Fix Adding Unexpected Conditions * Adds no_age field * Modify no_age description and variables name. Adds relevant comments. * Fixes Build Failure * Fixes indent issues and removes unnecessary condition. * Modified condition to include case when no_age is not present in the file.
* Fix Adding Unexpected Conditions * Adds no_age field * Modify no_age description and variables name. Adds relevant comments. * Fixes Build Failure * Fixes indent issues and removes unnecessary condition. * Modified condition to include case when no_age is not present in the file.
Fixes hashicorp/terraform-provider-google#14044