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

Jump to bottom

Fix labels perma diff #2386

Merged
merged 1 commit into from
Sep 18, 2024
Merged

Fix labels perma diff #2386

merged 1 commit into from
Sep 18, 2024

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Sep 16, 2024 •
edited
Loading

This PR takes advantage of pulumi/pulumi-terraform-bridge#2417 to fixup the incorrect planned state caused by empty labels.

Fixes #2372

Before merging

  • tests need to be added

@iwahbe iwahbe self-assigned this Sep 16, 2024
@github-actions GitHub Actions
Copy link

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

Maintainer note: consult the runbook for dealing with any breaking changes.

@iwahbe iwahbe force-pushed the iwahbe/fix-labels-perma-diff branch from f8d9702 to c9868fc Compare September 17, 2024 09:57
VenelinMartinov
VenelinMartinov reviewed Sep 17, 2024
View reviewed changes
provider/resources.go Outdated Show resolved Hide resolved
provider/resources.go Outdated Show resolved Hide resolved
provider/resources.go Outdated Show resolved Hide resolved
@iwahbe iwahbe force-pushed the iwahbe/fix-labels-perma-diff branch 2 times, most recently from dceb55c to 4230ada Compare September 17, 2024 11:57
@iwahbe iwahbe changed the title Iwahbe/fix labels perma diff Fix labels perma diff Sep 17, 2024
@iwahbe iwahbe force-pushed the iwahbe/fix-labels-perma-diff branch from 4230ada to 91f0ec4 Compare September 17, 2024 12:06
@iwahbe iwahbe marked this pull request as ready for review September 17, 2024 12:09
@iwahbe iwahbe force-pushed the iwahbe/fix-labels-perma-diff branch from 91f0ec4 to 094a682 Compare September 17, 2024 13:19
VenelinMartinov
VenelinMartinov reviewed Sep 17, 2024
View reviewed changes
Copy link
Contributor

@VenelinMartinov VenelinMartinov left a comment

Choose a reason for hiding this comment

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

Can we also add a test for unmanaged labels? You can use the gcloud CLI:

gcloud storage buckets update $bucket_name --update-labels=unmanaged=value,unmanaged_empty=

VenelinMartinov
VenelinMartinov reviewed Sep 17, 2024
View reviewed changes
provider/resources.go Outdated Show resolved Hide resolved
provider/resources.go Outdated Show resolved Hide resolved
VenelinMartinov
VenelinMartinov reviewed Sep 17, 2024
View reviewed changes
provider/resources.go Outdated Show resolved Hide resolved
provider/resources.go Outdated Show resolved Hide resolved
@VenelinMartinov
Copy link
Contributor

I don't think we are handling unmanaged labels correctly here. We should add a test for that case, both for empty and non-empty unmanaged labels.

@iwahbe iwahbe force-pushed the iwahbe/fix-labels-perma-diff branch from 094a682 to fcf1ebd Compare September 17, 2024 14:14
@iwahbe
Copy link
Member Author

iwahbe commented Sep 17, 2024

I don't think we are handling unmanaged labels correctly here. We should add a test for that case, both for empty and non-empty unmanaged labels.

It doesn't clober, though the upstream refresh behavior isn't great. I'll add a test and see if I can improve upstream's behavior.

@VenelinMartinov
Copy link
Contributor

Note, somewhat related: #1946 bucket has slightly different behaviour when importing labels.

@iwahbe
Copy link
Member Author

iwahbe commented Sep 17, 2024

I added a regression test, but I'm not going to tackle fixing labels in this PR. I opened #2390 to track general unmanaged labels work.

@VenelinMartinov
Copy link
Contributor

Can you please clarify what the actual problem is in #2390? Is it the effectiveLabels output changing during Refresh? If so, that seems expected.

@iwahbe
Copy link
Member Author

iwahbe commented Sep 17, 2024

Can you please clarify what the actual problem is in #2390? Is it the effectiveLabels output changing during Refresh? If so, that seems expected.

No, it's that it changes during the subsequent update, not during the refresh. I have clarified in the issue.

VenelinMartinov
VenelinMartinov reviewed Sep 17, 2024
View reviewed changes
Copy link
Contributor

@VenelinMartinov VenelinMartinov left a comment

Choose a reason for hiding this comment

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

Looks good - glad this worked out!

Two points here:

  1. Unit tests on the fixup function would be nice for whoever has to interact with this next - it helps to illustrate the desired behaviour as it is not obvious what the GCP provider returns when we interact with the Plan - happy to punt this for after fixing the issue if we want to ship sooner.
  2. Why do we not fix unmanaged labels? Why do we treat them differently?

@iwahbe iwahbe force-pushed the iwahbe/fix-labels-perma-diff branch from 761b856 to f128f7e Compare September 17, 2024 21:59
guineveresaenger
guineveresaenger reviewed Sep 18, 2024
View reviewed changes
Copy link
Contributor

@guineveresaenger guineveresaenger left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you for linking the outstanding work in the bridge and follow-up. A few clarifying requests here.

examples/examples_go_test.go Outdated Show resolved Hide resolved
provider/provider_yaml_test.go Show resolved Hide resolved
provider/resources.go Outdated Show resolved Hide resolved
provider/provider_yaml_test.go
@@ -1120,3 +1124,215 @@ func TestFirestoreDatabaseAutoname(t *testing.T) {
pt.SetConfig("gcpProj", proj)
pt.Up()
}

func TestEmptyLabels(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

How do we expect empty labels to behave? Can that be part of the test name?
a silly example - TestEmptyLabelsDefaultToCake - tells me Pulumi should rewrite all empty labels to say "cake".

provider/resources.go Show resolved Hide resolved
@guineveresaenger
Copy link
Contributor

Also - this needs pulumi/pulumi-terraform-bridge#2417 to merge and even release before we can merge here, right?

@VenelinMartinov
Copy link
Contributor

The issue with TestImport seems to be an engine regression, opened pulumi/pulumi#17290

@VenelinMartinov
Copy link
Contributor

Ran a few manual tests here:

  • Empty labels with the fix here can be created fine - this fixes the confusing behaviour around empty labels being dropped if they have no old value
  • Replacing a label with an empty value will keep the old value - this maintains the previous behaviour, which is somewhat unintuitive. I think this is probably fine.
  • Having a non-empty default and an empty label for the the same key will make an empty label - this seems sensible too - labels trump default labels, so an empty value there will take precedence.

@iwahbe
Fix empty labels perma-diff
9cd8120 
This PR takes advantage of pulumi/pulumi-terraform-bridge#2417 to fixup the incorrect
planned state caused by empty labels.

Fixes #2372
@iwahbe iwahbe force-pushed the iwahbe/fix-labels-perma-diff branch from 8a8d6c8 to 9cd8120 Compare September 18, 2024 14:26
@iwahbe iwahbe enabled auto-merge (rebase) September 18, 2024 14:27
@iwahbe iwahbe merged commit f8d6d51 into master Sep 18, 2024
23 checks passed
@iwahbe iwahbe deleted the iwahbe/fix-labels-perma-diff branch September 18, 2024 15:03
@pulumi-bot
Copy link
Contributor

This PR has been shipped in release v8.2.0.

@pulumi-bot pulumi-bot mentioned this pull request Sep 18, 2024
Closed
This was referenced Sep 20, 2024
Closed
Merged
VenelinMartinov added a commit that referenced this pull request Sep 25, 2024
@VenelinMartinov
Fix empty labels handling in Cluster resource (#2441)
c55b95a 
This fixes the empty label handling in the GCP Cluster resource.

In the fix for #2372
(#2386 and
pulumi/pulumi-terraform-bridge#2417) we did not
know that the labels property in GCP is sometimes overloaded, ex GCP
Custer.

For the Cluster resource, the GCP labels are under `resource_labels`,
not `labels`

This PR adds the logic to the empty labels fix and adds a regression
test.

fixes #2395
@VenelinMartinov VenelinMartinov mentioned this pull request Oct 8, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@VenelinMartinov VenelinMartinov VenelinMartinov left review comments

@guineveresaenger guineveresaenger guineveresaenger left review comments

Assignees

@iwahbe iwahbe

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Permadiff in pulumiLabels and effectiveLabels with empty labels
4 participants
@iwahbe @VenelinMartinov @guineveresaenger @pulumi-bot