-
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
appengine: Suppress null automatic_scaling blocks during Read #10476
appengine: Suppress null automatic_scaling blocks during Read #10476
Conversation
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 2 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAppEngineFlexibleAppVersion_update|TestAccAppEngineStandardAppVersion_update |
|
I think TestAccAppEngineFlexibleAppVersion_update failed because it and TestAccAppEngineStandardAppVersion_update fight over the same singleton resources and were run literally in parallel here. I'll update them. This should pass after /gcbrun |
Tests analyticsTotal tests: Click here to see the affected service packages
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 testsTestAccAppEngineFlexibleAppVersion_update |
|
Hmm- new error. /gcbrun |
Ah, I misread the initial failure. I misread an intended update call as the failing reason. The reason seems to be:
So unrelated to this change. Not sure why it's not failing in the nightlies then. |
Tests analyticsTotal tests: Click here to see the affected service packages
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 testsTestAccAppEngineFlexibleAppVersion_update |
|
/gcbrun |
Tests analyticsTotal tests: Click here to see the affected service packages
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 testsTestAccAppEngineStandardAppVersion_update |
|
...tes/terraform/custom_flatten/appengine_standardappversion_automatic_scaling_handlenil.go.erb
Show resolved
Hide resolved
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.
Not in love with the downsides of this maintenance-wise, but it seems better for the user experience.
...tes/terraform/custom_flatten/appengine_standardappversion_automatic_scaling_handlenil.go.erb
Show resolved
Hide resolved
...tes/terraform/custom_flatten/appengine_standardappversion_automatic_scaling_handlenil.go.erb
Show resolved
Hide resolved
Yeah- being in a oneof is the justification for taking this approach. |
Tests analyticsTotal tests: Click here to see the affected service packages
|
Fixes hashicorp/terraform-provider-google#17531
I went with this over O+C or a DSF because the field exists in a oneof (
scaling
in proto) and those options both preserve the set-but-deeply-nilautomatic_scaling
block in state, meaning we'd eventually send it to the server even if the user was sending a differentscaling
entry. This approach is worse if they start returning values for existing fields because it will break users again, but it's at least mitigatable with lifecycle.ignore_changes if that happens.Release Note Template for Downstream PRs (will be copied)