Skip to content
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.

Sign up for GitHub

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

Allow deletion of resource_bigquery_table when it still has associated resource tags #10568

Merged
merged 5 commits into from
May 7, 2024
Merged

Allow deletion of resource_bigquery_table when it still has associated resource tags #10568

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e2f1507
allow deletion of resource_bigquery_table when it still has associate…
wj-chen Apr 30, 2024
1409171
add a virtual field allow_resource_tags_on_deletion
wj-chen May 6, 2024
19a5db6
add new field to unit test config
wj-chen May 7, 2024
e30d3db
fix indentation
wj-chen May 7, 2024
49351c2
add version guard to import logic
wj-chen May 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions mmv1/products/bigquery/Table.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -516,3 +516,10 @@ properties:
in the namespaced format, for example "123456789012/environment" where 123456789012 is the
ID of the parent organization or project resource for this tag key. Tag value is expected
to be the short name, for example "Production".
virtual_fields:
- !ruby/object:Api::Type::Boolean
name: 'allow_resource_tags_on_deletion'
min_version: beta
description: |
Whether or not to allow table deletion when there are still resource tags attached.
default_value: false
26 changes: 21 additions & 5 deletions mmv1/third_party/terraform/services/bigquery/resource_bigquery_table.go.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1157,6 +1157,15 @@ func ResourceBigQueryTable() *schema.Resource {
Description: `Whether or not to allow Terraform to destroy the instance. Unless this field is set to false in Terraform state, a terraform destroy or terraform apply that would delete the instance will fail.`,
},

<% unless version == 'ga' -%>
"allow_resource_tags_on_deletion": {
Type: schema.TypeBool,
Optional: true,
Default: false,
Description: `Whether or not to allow table deletion when there are still resource tags attached.`,
},

<% end -%>
// TableConstraints: [Optional] Defines the primary key and foreign keys.
"table_constraints": {
Type: schema.TypeList,
Expand Down Expand Up @@ -1834,13 +1843,15 @@ func resourceBigQueryTableDelete(d *schema.ResourceData, meta interface{}) error
}
<% unless version == 'ga' -%>
if v, ok := d.GetOk("resource_tags"); ok {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a breaking change that should be avoided a minor release. This is Changing resource deletion behavior in https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/breaking-changes/#resource-level-breaking-changes.

We have a couple of options:

  1. wait until the next major release, which is expected in a couple of months
  2. introduce a virtual field to force delete the resource even when resource_tags are present and default it to false.

Please let me know your thoughts and if you have any questions. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the combination of the points below make it eligible for a minor version release?

  • Feature was very newly added (released on 4/22)
  • Feature is in the beta provider
  • The new deletion behavior is more permissive instead of more restrictive

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wj-chen Thanks for the quick response.

  • Feature was very newly added (released on 4/22)
  • Feature is in the beta provider

The beta provider is also a stable surface that does not allow breaking changes outside of major releases. See go/terraform-launch-stages#google-beta

  • The new deletion behavior is more permissive instead of more restrictive

I'd still think it's breaking since it's changing the default deletion behavior and I'm a bit hesitant about changing it directly in a minor. However, if an urgent fix is needed in a minor, we could follow https://googlecloudplatform.github.io/magic-modules/develop/breaking-changes/make-a-breaking-change/#in-minor-releases - "this change can be made if the default behavior stays the same and new behavior can be enabled with a flag", such as adding a force_delete field, which is the option 2 I proposed in #10568 (comment). Please let me know what your thought on this approach and if it will work for your use case. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a virtual field allow_resource_tags_on_deletion for this purpose and this PR is ready for review again.

Can you also describe the migration path from this to my original change? Would that be a candidate for the next major release?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will discuss internally regarding the migration path for the next major release

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @shuyama1, I'd like to follow up on this discussion again since the submission window for the next major release is now open. We want to promote this field to the GA provider, and make the default value true in the GA provider from the get-go if possible.

  • Would it count as a breaking change if the promotion and default value update take place together? If not, we'll proceed with that since it's the most ideal. Updating the default value for the beta provider will be treated as a separate breaking change.
  • Otherwise, we would most likely promote first then make a breaking change to update the default value for both providers as a follow-up. This way the GA promotion won't be blocked by the next major release timeline.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wj-chen Thank you for following up on this issue.

Our policy requires that the beta provider be a superset of the GA provider. This means the features and behaviors in GA should always be consistent with those in beta.

If we want to promote the feature to GA before changing the default in beta in 6.0.0 (as you correctly noted, this default value swap is a breaking change and must wait until the 6.0.0 major release), we need to use the current default value and update the default values for both GA and beta later in the 6.0.0 release.

Hope this answers your question and let me know if you have further questions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case we'll proceed with promoting the feature to GA first, then making a breaking change to change the default value for the field for both GA and beta providers in the 6.0.0 release. Thank you for the guidance!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect. Sounds good!

var resourceTags []string
if !d.Get("allow_resource_tags_on_deletion").(bool) {
var resourceTags []string

for k, v := range v.(map[string]interface{}) {
resourceTags = append(resourceTags, fmt.Sprintf("%s:%s", k, v.(string)))
}
for k, v := range v.(map[string]interface{}) {
resourceTags = append(resourceTags, fmt.Sprintf("%s:%s", k, v.(string)))
}

return fmt.Errorf("cannot destroy table %v without clearing the following resource tags: %v", d.Id(), resourceTags)
return fmt.Errorf("cannot destroy table %v without unsetting the following resource tags or setting allow_resource_tags_on_deletion=true: %v", d.Id(), resourceTags)
}
}

<% end -%>
Expand Down Expand Up @@ -2675,6 +2686,11 @@ func resourceBigQueryTableImport(d *schema.ResourceData, meta interface{}) ([]*s
if err := d.Set("deletion_protection", true); err != nil {
return nil, fmt.Errorf("Error setting deletion_protection: %s", err)
}
<% unless version == 'ga' -%>
if err := d.Set("allow_resource_tags_on_deletion", false); err != nil {
return nil, fmt.Errorf("Error setting allow_resource_tags_on_deletion: %s", err)
}
Comment on lines +2690 to +2692
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may also need to mark it as beta only.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

<% end -%>

// Replace import id for the resource id
id, err := tpgresource.ReplaceVars(d, config, "projects/{{project}}/datasets/{{dataset_id}}/tables/{{table_id}}")
Expand Down
9 changes: 6 additions & 3 deletions mmv1/third_party/terraform/services/bigquery/resource_bigquery_table_test.go.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1579,7 +1579,7 @@ func TestAccBigQueryTable_ResourceTags(t *testing.T) {
ResourceName: "google_bigquery_table.test",
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"deletion_protection"},
ImportStateVerifyIgnore: []string{"deletion_protection", "allow_resource_tags_on_deletion"},
},
{
Config: testAccBigQueryTableWithResourceTagsUpdate(context),
Expand All @@ -1588,7 +1588,7 @@ func TestAccBigQueryTable_ResourceTags(t *testing.T) {
ResourceName: "google_bigquery_table.test",
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"deletion_protection"},
ImportStateVerifyIgnore: []string{"deletion_protection", "allow_resource_tags_on_deletion"},
},
// testAccBigQueryTableWithResourceTagsDestroy must be called at the end of this test to clear the resource tag bindings of the table before deletion.
{
Expand All @@ -1598,7 +1598,7 @@ func TestAccBigQueryTable_ResourceTags(t *testing.T) {
ResourceName: "google_bigquery_table.test",
ImportState: true,
ImportStateVerify: true,
ImportStateVerifyIgnore: []string{"deletion_protection"},
ImportStateVerifyIgnore: []string{"deletion_protection", "allow_resource_tags_on_deletion"},
},
},
})
Expand Down Expand Up @@ -4001,6 +4001,7 @@ resource "google_bigquery_table" "test" {
provider = google-beta

deletion_protection = false
allow_resource_tags_on_deletion = true
dataset_id = "${google_bigquery_dataset.test.dataset_id}"
table_id = "%{table_id}"
resource_tags = {
Expand Down Expand Up @@ -4050,6 +4051,7 @@ resource "google_bigquery_table" "test" {
provider = google-beta

deletion_protection = false
allow_resource_tags_on_deletion = true
dataset_id = "${google_bigquery_dataset.test.dataset_id}"
table_id = "%{table_id}"
resource_tags = {
Expand Down Expand Up @@ -4100,6 +4102,7 @@ resource "google_bigquery_table" "test" {
provider = google-beta

deletion_protection = false
allow_resource_tags_on_deletion = true
dataset_id = "${google_bigquery_dataset.test.dataset_id}"
table_id = "%{table_id}"
resource_tags = {}
Expand Down