-
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
Enforce mutual exclusivity among view, materialized view, and schema in BigQuery table config #7973
Enforce mutual exclusivity among view, materialized view, and schema in BigQuery table config #7973
Conversation
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @melinath, 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. |
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, 83 insertions(+), 29 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 testsTestAccSqlUser_postgresIAM|TestAccSqlUser_postgres|TestAccSqlUser_postgresAbandon|TestAccComputeFirewallPolicyRule_multipleRules|TestAccBigQueryTable_MaterializedView_WithSchema|TestAccBigQueryTable_WithViewAndSchema|TestAccAlloydbCluster_missingLocation|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccAlloydbBackup_missingLocation|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccDataSourceAlloydbLocations_basic |
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.
Sorry for the delayed review - lost track of this. I'm concerned about this implementation because it seems like it is intentionally not updating the schema field.
After a user runs terraform apply
, the API resource should match what is in the Terraform configuration - so the schema field also needs to be set properly during create for Terraform, even though the API only supports setting it via update. From the ticket, it sounds like the problem was that the update may not have worked because of a problem on the API side? Please let me know if you think I'm missing something :-)
Oops I requested a re-review before seeing your comment. Thanks for taking a look! In short, "view"/"materialized_view" and "schema" should be mutually exclusive, hence the (unchanged in this PR) behavior of setting "schema" to nil for the create request. If "schema" is specified when "view"/"materialized_view" is present, it should be ignored, hence the change in this PR (there is a buganizer issue describing why that needs to be, can I link it here?). So in this case, by nature, the resource to be created and what's shown in the Terraform config will differ. If it's more in line with the style standard - instead of ignoring, we can outright fail the |
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.
more context at b/280326829 (yes, fine to share - happy to move discussion there if needed)
If the fields are mutually exclusive (not just needing an additional update call), you can add ConflictsWith
to the schema to make sure that users are alerted as early as possible about the conflict. It will show up as a validation error at plan time.
That would be preferred over silently not sending the schema and then having a diff, since the configuration is essentially asking for something invalid.
Adding ConflictsWith would generally be considered a breaking change, and would therefore need to be held for the next major release, because it could cause a configuration that previously applied successfully to start failing.
However, if an invalid configuration would always cause the API to return an error (so that the configuration could never be successfully applied) or if it would always cause a permadiff (especially a destructive permadiff) then we may be able to make an exception and include the change in a minor release.
I'll need to investigate this more closely myself before merge, but I wanted to get your input on whether you think it would be better to wait for a major release vs. trying to move forward now based on what I said above.
Thank you for the pointer. ConflictsWith looks very promising and I'll look into it. As mentioned in the bug, throwing a validation error in this case is the current behavior of other clients e.g. CLI.
I think this can wait until the next major release. The extraneous update call may or may not fail depending on the exact schema, so it's not an always-return-an-error situation, but I'd like to understand what you menat by "permadiff"? Also general question about release cadence - for a newly submitted change, how long do we expect it to be released to terraform-google-providers and terraform-google-providers-beta as part of a minor or major release? |
regarding the questions - I'll reach out internally. |
Update: I was able to verify locally the desired behavior using |
Generally we don't add tests to make sure that ConflictsWith is honored by Terraform. But you could probably test it with ExpectError if you want to. This would be best as part of an existing test if bigquery resources take a while to set up. |
Thanks again! I updated this PR and included new tests. Since the new tests are expected to fail in the schema validation stage, they shouldn't take a while to set up since they don't actually create any resource. |
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, 155 insertions(+), 49 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 11 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccSqlUser_postgresAbandon|TestAccSqlUser_postgres|TestAccSqlUser_postgresIAM|TestAccBillingSubaccount_basic|TestAccBillingSubaccount_renameOnDestroy|TestAccDataSourceGoogleFirebaseAndroidAppConfig|TestAccComputeFirewallPolicyRule_multipleRules|TestAccBigQueryTable_MaterializedView_WithView|TestAccApigeeKeystoresAliasesPkcs12_ApigeeKeystoresAliasesPkcs12Example|TestAccApigeeKeystoresAliasesKeyCertFile_apigeeKeystoresAliasesKeyCertFileTestExample|TestAccDataSourceAlloydbLocations_basic |
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 - this will be much more clear! I've added the referenced ticket to the 5.0.0 milestone. Please reach out at the end of July to get this merged.
…in BigQuery table config
I created a new PR with this change on the major release feature branch: #8494 |
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.
Please resolve the conflicts in mmv1/third_party/terraform/resources/resource_bigquery_table.go - you could do this by rebasing onto the major release branch or be merging the major release branch into this branch.
Note - I've changed the base branch of this PR. Since the PR is opened from your main branch, you'll need to make all updates for this PR to your main branch. In the future I'd recommend creating new branches as described in GitHub flow so that your main branch (or any feature release branches) can track the related branch in this repository.
I need to clean up my fork of the repo. Will ping here again when it's ready. |
d09eab9
to
fe1cca8
Compare
Thanks for the resources! Will do once this is merged. |
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, 159 insertions(+), 49 deletions(-)) |
Tests analyticsTotal tests:
|
Manual test run since these can't run in VCR: https://ci-oss.hashicorp.engineering/buildConfiguration/GoogleCloud_ProviderGoogleCloudMmUpstream/426090?hideProblemsFromDependencies=false&hideTestsFromDependencies=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.
test passed in CI, lgtm
87e4f29
into
GoogleCloudPlatform:FEATURE-BRANCH-major-release-5.0.0
Please be sure to add this to the version 5 upgrade guide! Thanks! |
Tests analyticsTotal tests:
|
Thank you very much! Will get on that next. |
Upgrade doc update has been created at #8607. |
…in BigQuery table config (GoogleCloudPlatform#7973) * Enforce mutual exclusivity among view, materialized view, and schema in BigQuery table config * fix merge conflict * fix field specification and add a VCR skip for the new acceptance test * skip VCR for MaterializedView_WithView test too
… schema in BigQuery table config (GoogleCloudPlatform#7973)" This reverts commit 87e4f29.
… schema in BigQuery table config (GoogleCloudPlatform#7973)" This reverts commit 87e4f29.
… schema in BigQuery table config (GoogleCloudPlatform#7973)" This reverts commit 87e4f29.
Fixes hashicorp/terraform-provider-google#14220.
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)