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

PlanResourceChange dirty refresh on empty maps #2047

Closed
VenelinMartinov opened this issue May 31, 2024 · 11 comments
Closed

PlanResourceChange dirty refresh on empty maps #2047

VenelinMartinov opened this issue May 31, 2024 · 11 comments
Assignees
Labels
kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed

Comments

@VenelinMartinov
Copy link
Contributor

VenelinMartinov commented May 31, 2024

What happened?

On master with PlanResourceChange enabled it looks like empty maps now refresh badly.

Example

TestAccBucketPy

https://github.com/pulumi/pulumi-gcp/actions/runs/9215462858/job/25354722934

Output of pulumi about

.

Additional context

No response

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@VenelinMartinov VenelinMartinov added needs-triage Needs attention from the triage team kind/bug Some behavior is incorrect or out of spec and removed needs-triage Needs attention from the triage team labels May 31, 2024
@VenelinMartinov VenelinMartinov self-assigned this May 31, 2024
@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Jun 6, 2024

This doesn't repro in TF, so likely a bridge issue

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented Jun 6, 2024

Ah, this actually reproes in TF but TF does not show it to the user:

after the first apply we get labels: nil in the state

and after the refresh it changes to labels: {}

But it doesn't show anything about it on the CLI - I was using terraform refresh which is apparently deprecated in favour of --refresh-only

@VenelinMartinov
Copy link
Contributor Author

Ok it does show it but with terraform plan --refresh-only:

esc run providers.all -- terraform plan --refresh-only
google_storage_bucket.bucket: Refreshing state... [id=example-bucket123123223]

Note: Objects have changed outside of Terraform

Terraform detected the following changes made outside of Terraform since the
last "terraform apply" which may have affected this plan:

  # google_storage_bucket.bucket has changed
  ~ resource "google_storage_bucket" "bucket" {
        id                          = "example-bucket123123223"
      + labels                      = {}
        name                        = "example-bucket123123223"
        # (15 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }


This is a refresh-only plan, so Terraform will not take any actions to undo
these. If you were expecting these changes then you can apply this plan to
record the updated values in the Terraform state without changing any remote
objects.

─────────────────────────────────────────────────────────────────────────────

Note: You didn't use the -out option to save this plan, so Terraform can't
guarantee to take exactly these actions if you run "terraform apply" now.

@VenelinMartinov
Copy link
Contributor Author

Some potential ways around this:

  1. Accept the diff as correct in the bridge and patch each provider to ignore diffs in empty/nil maps where relevant
  2. Restore the non-PRC behaviour which saved nil maps as empty, purposefully diverging from the way TF handles these inputs. Could be problematic for cases where nil maps are meaningful.
  3. Add some thing for diffing maps?

@VenelinMartinov
Copy link
Contributor Author

@VenelinMartinov
Copy link
Contributor Author

as suggested by @t0yv0 pulumi/pulumi#16146 fixes this

@VenelinMartinov
Copy link
Contributor Author

Also explored in #2063 by @t0yv0

@VenelinMartinov
Copy link
Contributor Author

Possibly related? #1106

@t0yv0
Copy link
Member

t0yv0 commented Jun 6, 2024

Not sure if 1106 is very related, it's read returning null when we want it not to, here it's the reverse. The workaround I would like to try here is not likely to solve 1106 though I can test easily.

@t0yv0
Copy link
Member

t0yv0 commented Jun 7, 2024

Got pulumi/pulumi-aws#4013 to get this fixed for AWS. Curious if other providers satisfied as well.

VenelinMartinov added a commit that referenced this issue Jun 11, 2024
Related to #2047

Pulling out the tests from
#2065 so that the
changes in behaviour in the other PR are more visible and easy to
review.

This adds tests for refreshes on collections for:
- list/set/map
- RPC/non-PRC
- null/empty/non-empty collections
- nil/non-nil cloud values for the nil collections
- top-level collections/nested collections
- Properties with a value override in Create/ ones without - this is a
behaviour observed in AWS and GCP labels and some other providers,
mainly seen around maps.

The goal of the tests is to:
- Ensure we do not regress on behaviour in PRC vs non-PRC
- Ensure we do not regress on behaviour in PRC as we change the
implementation in
#2073 /
#2065

I've flattened the matrix in order to allow us to annotate/skip
individual test cases.
@VenelinMartinov VenelinMartinov added the resolution/fixed This issue was fixed label Jun 24, 2024
@VenelinMartinov
Copy link
Contributor Author

This is no longer an issue with the new refresh behaviour in the CLI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Some behavior is incorrect or out of spec resolution/fixed This issue was fixed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants