fix: (storage) fix inconsistent plan issue for custome_attributes field

[gurusai-voleti]

Release Note Template for Downstream PRs (will be copied)

See Write release notes for guidance.
Fixes: hashicorp/terraform-provider-google#26804

storage: fix inconsistent plan issue for `custome_attributes` field

[github-actions]

Googlers: For automatic test runs see go/terraform-auto-test-runs.

@roaks3, 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.

[modular-magician]

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.

google provider: Diff ( 2 files changed, 22 insertions(+), 7 deletions(-))
google-beta provider: Diff ( 2 files changed, 22 insertions(+), 7 deletions(-))

[modular-magician]

Tests analytics

Total tests: 133
Passed tests: 122
Skipped tests: 11
Affected tests: 0

Click here to see the affected service packages

• storage

🟢 All tests passed!

View the build log

[github-actions]

@roaks3 This PR has been waiting for review for 3 weekdays. Please take a look! Use the label disable-review-reminders to disable these notifications.

[roaks3]

From a teammate:

this looks like a case where the inconsistency on plan can be resolved with a planModifier

https://developer.hashicorp.com/terraform/plugin/framework/resources/plan-modification#attribute-plan-modification

[gurusai-voleti]

From a teammate:

this looks like a case where the inconsistency on plan can be resolved with a planModifier
https://developer.hashicorp.com/terraform/plugin/framework/resources/plan-modification#attribute-plan-modification

after adding planmodifiers also, when running tf plan/apply shows changes even no change is required and this is not fully solving the issues

the change provided in the PR is sufficient to solve the issue, please let me know your point of view

[roaks3]

I think the suggestion is to use plan modifiers as the more idiomatic approach, unless there is an issue with doing so. Otherwise, we end up with a proliferation of custom solutions, which is far more difficult to maintain.

Could you share any more about what you tried and what did not work?

Looping in @BBBmau who has more expertise with the framework.

[BBBmau]

after going through the changes, this is the correct implementation since a modifyPlan requires checking the values between current state and expected change.

Going from {} -> null isn't exactly possible since it causes a type mismatch:

│ Error: Provider produced invalid plan
│ 
│ Provider "registry.terraform.io/hashicorp/google" planned an invalid value for google_storage_notification.notification.custom_attributes: planned value cty.NullVal(cty.Map(cty.String))
│ does not match config value cty.MapValEmpty(cty.String).

Though i question whether we want to do this at all. Having this included results in a force change when explicitly setting custom_attributes = {} when "custom_attributes": null

Terraform will perform the following actions:

  # google_storage_notification.notification must be replaced
-/+ resource "google_storage_notification" "notification" {
      + custom_attributes = {} # forces replacement
      ~ id                = "testing-custom-attributes/notificationConfigs/33" -> (known after apply)
      ~ notification_id   = "33" -> (known after apply)
      ~ self_link         = "https://www.googleapis.com/storage/v1/b/testing-custom-attributes/notificationConfigs/33" -> (known after apply)
        # (4 unchanged attributes hidden)
    }

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

Not merging this would keep the inconsistent apply which prevents this from being recreated. Though I also noticed that the state gets tainted due to the destroy step being completed followed by recreation which is where we then experience the inconsistent result after apply.

I've attempted to look for any patches that can be done we prevent the force recreation when going from {} -> null in state but it looks to be a limitation in framework.

Since the force recreation is already a behavior that is present on this behavior we can move forward with this fix for the inconsistent result after apply.

cc: @roaks3 I'll open a issue on plugin-framework to see if there's something that can be done to resolve this specific diff case.

[roaks3]

Thanks @BBBmau !