-
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
Add resource tags to BigQuery Table #10455
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @rileykarson, 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.
|
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 testsTestAccBigQueryTable_ResourceTags |
|
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 verified the account has the appropriate permissions, so that error message is probably an effective 404 unless hierarchical permissions aren't getting respected here.
(I haven't taken a further look at the change itself yet)
Thank you for checking the permissions! It turned out to be a race condition where resources were referenced before creation, which should be fixed by the newest commit. |
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 testsTestAccBigQueryTable_ResourceTags |
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.
Generally LGTM- one question on the deletion precondition before approving/merging, and a couple nits inline.
mmv1/third_party/terraform/services/bigquery/resource_bigquery_table.go.erb
Outdated
Show resolved
Hide resolved
@@ -1809,6 +1840,18 @@ func resourceBigQueryTableDelete(d *schema.ResourceData, meta interface{}) error | |||
if d.Get("deletion_protection").(bool) { | |||
return fmt.Errorf("cannot destroy instance without setting deletion_protection=false and running `terraform apply`") | |||
} | |||
<% unless version == 'ga' -%> | |||
if v, ok := d.GetOk("resource_tags"); ok { |
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.
Does the API have this check / does it take a long time to report a failure? Otherwise, if the API will reject the call there's not much value in doing this clientside first, since this will save 1s / one API call AIUI.
I'm a little hesitant about just checking state here- Terraform will sometimes short-circuit delete calls so Terraform may have drifted from the real config. I'm not sure we ever drift from state though, it's been a bit since I knew all the rules.
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.
The deletion will go through actually, but what I discovered while debugging the VCR test failure is that if a table is deleted while having resource tags specified, it's impossible to delete the tag value resource, failing at "it's still has a binding". As a follow-up I'll reach out to our API team to discuss if the behavior should be fixed on the API-level.
mmv1/third_party/terraform/services/bigquery/resource_bigquery_table.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.
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
Thanks for the review! The new revision is out. Hoping to get this merged before the release cut today. |
Adds support for resource tags to BigQuery Table.
Release Note Template for Downstream PRs (will be copied)