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.

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

switch deletion_policy to virtual field for google_firebase_web_app #6738

Merged
merged 9 commits into from
Oct 25, 2022

Conversation

ScottSuarez
Copy link
Contributor

@ScottSuarez ScottSuarez commented Oct 24, 2022

Fix for issues caused by #6652

This new field was forcing recreation due to not being set on READ.

abandon default forcing recreation.

  # google_firebase_web_app.skip_delete must be replaced
-/+ resource "google_firebase_web_app" "skip_delete" {
      ~ app_id          = "1:82023037933:web:5ec7d3d6a16d2b322945e4" -> (known after apply)
      + deletion_policy = "ABANDON" # forces replacement
      ~ id              = "projects/scottsuarez-graphite/webApps/1:82023037933:web:5ec7d3d6a16d2b322945e4" -> (known after apply)
      ~ name            = "projects/scottsuarez-graphite/webApps/1:82023037933:web:5ec7d3d6a16d2b322945e4" -> (known after apply)
        # (2 unchanged attributes hidden)
    }

Plan: 1 to add, 0 to change, 1 to destroy.

Although, even adding a value with a default will break configurations since we don't read this value anywhere. This leads me to believe adding any value that has a default will encounter this unless it's set on read if the api doesn't return it. It looks like the normal generator save url-param-only already accounts for this.

Terraform will perform the following actions:

  # google_firebase_web_app.skip_delete will be updated in-place
  ~ resource "google_firebase_web_app" "skip_delete" {
      + deletion_policy = "ABANDON"
        id              = "projects/scottsuarez-graphite/webApps/1:82023037933:web:5ec7d3d6a16d2b322945e4"
        name            = "projects/scottsuarez-graphite/webApps/1:82023037933:web:5ec7d3d6a16d2b322945e4"
        # (4 unchanged attributes hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Switching to a virtual field resolved this.

No changes. Your infrastructure matches the configuration.

Release Note Template for Downstream PRs (will be copied)


@ScottSuarez ScottSuarez changed the title switch deletion_policy to virtual field switch deletion_policy to virtual field for google_firebase_web_app Oct 24, 2022
@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 65 files changed, 82 insertions(+), 81 deletions(-))
Terraform Beta: Diff ( 74 files changed, 97 insertions(+), 99 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 65 files changed, 82 insertions(+), 81 deletions(-))
Terraform Beta: Diff ( 74 files changed, 97 insertions(+), 99 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

Copy link
Member

@rileykarson rileykarson left a comment

Choose a reason for hiding this comment

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

I just wanted to call to attention that this change makes the field updatable. I think that's a positive thing- there's no reason it had to recreate the resource in the first place, and recreation would have actually been undesirable imo. That meant there was no way to change the deletion policy without doing a deletion!

It looks like the normal generator save url-param-only already accounts for this.

Fields present in the API aren't a problem because their values get read during the refresh prior to plan. They do actually cause issues if you do a provider upgrade and then a refreshless plan, but that's an uncommon move and the fix is expensive (doing a lot of state upgrades).

url_param_only is really meant to serve a specific purpose, fields in the URL that will never change, but the functionality is useful so it got used for other purposes. That was part of the motivation for introducing the virtual field override in the first place!

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2207
Passed tests 1966
Skipped tests: 239
Failed tests: 2

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests
TestAccFirebaserulesRelease_BasicRelease|TestAccFirebaseWebApp_firebaseWebAppBasicExample

@modular-magician
Copy link
Collaborator

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]

Tests failed during RECORDING mode:
TestAccFirebaseWebApp_firebaseWebAppBasicExample[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 14 files changed, 82 insertions(+), 26 deletions(-))
Terraform Beta: Diff ( 16 files changed, 97 insertions(+), 36 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2207
Passed tests 1966
Skipped tests: 239
Failed tests: 2

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests
TestAccFirebaserulesRelease_BasicRelease|TestAccFirebaseWebApp_firebaseWebAppBasicExample

@modular-magician
Copy link
Collaborator

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]

Tests failed during RECORDING mode:
TestAccFirebaseWebApp_firebaseWebAppBasicExample[Error message] [Debug log]

Please fix these to complete your PR
View the build log or the debug log for each test

@modular-magician
Copy link
Collaborator

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 14 files changed, 82 insertions(+), 26 deletions(-))
Terraform Beta: Diff ( 16 files changed, 97 insertions(+), 36 deletions(-))
TF Validator: Diff ( 2 files changed, 3 insertions(+), 3 deletions(-))

description: |
Set to `ABANDON` to allow the WebApp to be untracked from terraform state
rather than deleted upon `terraform destroy`. This is useful becaue the WebApp may be
serving traffic. Set to `DELETE` to delete the WebApp. Default to `ABANDON`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
serving traffic. Set to `DELETE` to delete the WebApp. Default to `ABANDON`
serving traffic. Set to `DELETE` to delete the WebApp. Defaults to `ABANDON`.

@@ -54,6 +54,15 @@ overrides: !ruby/object:Overrides::ResourceOverrides
org_id: :ORG_ID
ignore_read_extra:
- project
- deletion_policy
Copy link
Member

Choose a reason for hiding this comment

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

We may want to call out something about importing the field in the docs, i.e.

Upon import, this field will always take on the default value of "ABANDON", regardless of what's specified in configuration. If you have another value set, a diff will appear in terraform plan. Run terraform apply to apply that value.

Copy link
Member

Choose a reason for hiding this comment

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

this seems like not a great experience - would this be possible for us to fix / do we already have a ticket open to fix it?

Copy link
Member

Choose a reason for hiding this comment

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

@modular-magician
Copy link
Collaborator

Tests analytics

Total tests: 2207
Passed tests 1967
Skipped tests: 239
Failed tests: 1

Action taken

Triggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed tests
TestAccFirebaserulesRelease_BasicRelease

@modular-magician
Copy link
Collaborator

Tests passed during RECORDING mode:
TestAccFirebaserulesRelease_BasicRelease[Debug log]

All tests passed
View the build log or the debug log for each test

@rileykarson
Copy link
Member

fyi @melinath: We'll want to hold/cherry-pick for this fix, or revert the original change

@ScottSuarez ScottSuarez merged commit adefb62 into GoogleCloudPlatform:main Oct 25, 2022
@ScottSuarez ScottSuarez deleted the delete-policy branch October 25, 2022 22:58
rainshen49 added a commit to rainshen49/magic-modules that referenced this pull request Dec 5, 2022
slevenick pushed a commit that referenced this pull request Feb 2, 2023
* Make deletion_policy a virtual_field for AndroidApp and AppleApp similar to #6738

* Accomodate upstream comments and inconsistent patches
kubalaguna pushed a commit to kubalaguna/magic-modules that referenced this pull request Feb 27, 2023
…gleCloudPlatform#6911)

* Make deletion_policy a virtual_field for AndroidApp and AppleApp similar to GoogleCloudPlatform#6738

* Accomodate upstream comments and inconsistent patches
ericayyliu pushed a commit to ericayyliu/magic-modules that referenced this pull request Jul 26, 2023
…gleCloudPlatform#6911)

* Make deletion_policy a virtual_field for AndroidApp and AppleApp similar to GoogleCloudPlatform#6738

* Accomodate upstream comments and inconsistent patches
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants