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

Fixes empty tag handling #2944

Merged
merged 40 commits into from
Nov 7, 2023
Merged

Fixes empty tag handling #2944

merged 40 commits into from
Nov 7, 2023

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Oct 31, 2023

Fixes #2895

Requires a bridge change, currently still pre-release: pulumi/pulumi-terraform-bridge@v3.63.2...1e955db

Due to confusing empty values with missing values at several layers of the stack, working with empty tag values was problematic before this change both in the Pulumi provider and in the upstream provider; this is now fixed by:

  • removing SetTagsDiff customizer from upstream; it is not working correctly for empty tags
  • similarly, removing tags_all computation for Plugin Framework based resources
  • completely taking over tagsAll handling from the TF layer to Pulumi layer; tagsAll is no longer marked as computed when working with TF (TF no longer sees it as computed), instead Pulumi computes tagsAll as a copy of tags and passes it down
  • fixing a bridge bug that ignored adding an empty tag (incorrect DIFF_NONE)

This is change is now passing an aggressive random-sampled state transition matrix for various combinations of provider-level and resource-level tags.

@t0yv0 t0yv0 force-pushed the t0yv0/fix-empty-tag-handling branch from 6d4a839 to 1b96168 Compare October 31, 2023 18:21
Copy link

github-actions bot commented Nov 1, 2023

Does the PR have any schema changes?

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.

@t0yv0
Copy link
Member Author

t0yv0 commented Nov 1, 2023

The test failures indicate that PF support is still required specially; and that this regressed on handling unknown and secrets in tags.

@t0yv0 t0yv0 marked this pull request as ready for review November 6, 2023 17:14
@t0yv0 t0yv0 requested review from iwahbe and a team November 6, 2023 17:14
Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

This looks good to me. I have 2 quick questions then lets merge.

provider/go.mod Show resolved Hide resolved
scripts/patch_computed_only.sh Outdated Show resolved Hide resolved
@t0yv0
Copy link
Member Author

t0yv0 commented Nov 7, 2023

I'm deferring the automation suggestion in #2962 - it's very valuable and borderline critical but this fix has been late in coming and I do not want to block on that work.

provider/tags_test.go Show resolved Hide resolved
provider/tags_test.go Show resolved Hide resolved
@t0yv0
Copy link
Member Author

t0yv0 commented Nov 7, 2023

@iwahbe PTAL. This is passing again.

Copy link
Member

@iwahbe iwahbe left a comment

Choose a reason for hiding this comment

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

LGTM

I think we will need to prioritize #2962, since this patch seems relatively conflict prone. That said, the fix itself looks good.

@t0yv0
Copy link
Member Author

t0yv0 commented Nov 7, 2023

Yeah!

@t0yv0 t0yv0 merged commit 4c3960c into master Nov 7, 2023
16 checks passed
@t0yv0 t0yv0 deleted the t0yv0/fix-empty-tag-handling branch November 7, 2023 23:42
t0yv0 added a commit that referenced this pull request Nov 29, 2023
Fixes #3064 

There is an issue with users being unable to add tags to AWS Firehose
delivery stream and select other resources.

As part of the #2944 mitigation
the provider needs to make sure that tags_all attributes, if present,
are not Computed because pulumi-aws will set them before passing down to
TF code.

TestTagsAllNoLongerComputed was supposed to check this.

Unfortunately the transformation missed that some resources define
SchemaFunc instead of Schema, and so did the test. The test is made
stricter here, and the underlying transformation is fixed.
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.

Incorrect tags applied when tag is empty
3 participants