-
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
Allows empty value and sets default value for maxRetries of cloud run job #7425
Conversation
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, 2 insertions(+), 2 deletions(-)) |
This will make it so that when maxRetries is not set by the user in their config we will send the value of Is that an acceptable behavior? |
Tests analyticsTotal tests: Action takenFound 9 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccCloudRunV2Job_cloudrunv2JobBasicExample|TestAccCloudRunV2Job_cloudrunv2JobVpcaccessExample|TestAccCloudRunV2Job_cloudrunv2JobSqlExample|TestAccComposerEnvironment_withEncryptionConfigComposer2|TestAccCloudRunV2Job_cloudrunv2JobSecretExample|TestAccCloudRunV2Job_cloudrunv2JobFullUpdate|TestAccCloudRunV2JobIamPolicyGenerated|TestAccCloudRunV2JobIamMemberGenerated|TestAccCloudRunV2JobIamBindingGenerated |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
No. What I want is when users set maxRetries to 0, it will be sent to the API instead of being ignored. |
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, 2 insertions(+), 2 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 13 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccNetworkServicesGateway_update|TestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstanceFromRegionTemplate_basic|TestAccComposerEnvironment_withEncryptionConfigComposer2|TestAccCloudRunV2Job_cloudrunv2JobVpcaccessExample|TestAccCloudRunV2Job_cloudrunv2JobSecretExample|TestAccCloudRunV2Job_cloudrunv2JobFullUpdate|TestAccDataSourceDNSKeys_basic|TestAccDataSourceDNSKeys_noDnsSec|TestAccFrameworkProviderBasePath_setBasePath|TestAccFrameworkProviderMeta_setModuleName|TestAccDataSourceDnsRecordSet_basic|TestAccDataSourceDnsManagedZone_basic |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Added |
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.
One small change, other than that it looks fine
/gcbrun |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 12 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccCloudRunV2Job_cloudrunv2JobBasicExample|TestAccCloudRunV2Job_cloudrunv2JobSecretExample|TestAccCloudRunV2Job_cloudrunv2JobFullUpdate|TestAccCloudRunV2Job_cloudrunv2JobVpcaccessExample|TestAccCloudRunV2Job_cloudrunv2JobSqlExample|TestAccComposerEnvironment_withEncryptionConfigComposer2|TestAccApigeeEnvKeystoreAliasPkcs12_apigeeEnvKeystoreAliasPkcs12Example|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccCloudRunV2JobIamBindingGenerated|TestAccCloudRunV2JobIamPolicyGenerated|TestAccCloudRunV2JobIamMemberGenerated|TestAccDataSourceGoogleFirebaseAndroidAppConfig |
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. Adding @rileykarson as a secondary reviewer because this is technically a breaking change
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- as discussed offline this may result in a diff in limited circumstances: configurations that have set a non-default value and then been updated to remove the field. Restoring drift detection is generally not considered to be a breaking change, although this is not a kind of change we want to be making regularly.
… job (GoogleCloudPlatform#7425) * send empty value * add client side default * remove defaul_from_api
… job (GoogleCloudPlatform#7425) * send empty value * add client side default * remove defaul_from_api
… job (GoogleCloudPlatform#7425) * send empty value * add client side default * remove defaul_from_api
… job (GoogleCloudPlatform#7425) * send empty value * add client side default * remove defaul_from_api
Fixes hashicorp/terraform-provider-google#13747.
Got compiling error when running tests in downstream repos, which was not caused by this change.
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)