-
Notifications
You must be signed in to change notification settings - Fork 155
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
Compute "tags"
in Check
#2655
Compute "tags"
in Check
#2655
Conversation
This will show diffs on |
Currently, this works for:
Read is the only CRUD+ method that does not go through Check, so it will also need special behavior so that |
I was not able to isolate this, do you have a repro? Can we file a bug upstream with a repro? |
How can we demonstrate that import and refresh scenarios exercising Read work as expected? |
What happens if a provider with this set of changes runs against stacks that used the old method and perhaps populated tags_all? |
Overall I appreciate this approach, if we cannot find a clean way to interface with the functionality in upstream provider, maintaining custom code around default tags seems worth it as it is a feature impacting every resource; if we can do this cleanly. There is a cost of doing so in maintaining this new code and ensuring that it does not interact in weird ways with the upstream machinery that attempts to solve the problem as well. |
By adding a test that exercises the behavior. We can't do that yet, since any such test will fail until pulumi/pulumi-terraform-bridge#1312 merges. |
They will see a diff on |
Ah super, you have a way to demonstrate in a test, that's great, let's try it. I think you can make the CI exercise pre-release code via go mod-replace. Alternatively, I had some nits on pulumi/pulumi-terraform-bridge#1312 but it's also mergeable once we add a quick test in there. |
62c00f7
to
edf865e
Compare
edf865e
to
304f439
Compare
304f439
to
1f1737c
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
05a87ac
to
16f0081
Compare
Having a look at this again shortly. |
diff --git a/internal/service/s3legacy/bucket_legacy.go b/internal/service/s3legacy/bucket_legacy.go | ||
index 0563bd3a25..e6bd8e7013 100644 | ||
--- a/internal/service/s3legacy/bucket_legacy.go | ||
+++ b/internal/service/s3legacy/bucket_legacy.go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to just combine this into, or put this this change alongside patch 003 where we add this code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting. The patch is still needed it just got shorter?
@@ -0,0 +1,217 @@ | |||
module github.com/pulumi/pulumi-aws/provider/tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK with introducing a test-specific module? I've done it elsewhere but was wondering if that's idiomatic Go for the problem of isolating test dependencies.
I had a question about the semantic of one of the tests, and I'm not familiar with the underlying patch being edited. Otherwise LGTM. |
725c1f9
to
fa2587d
Compare
TF does, and if we assume that "tags" is set correctly, we then get the expected diff.
This fix takes effect during Check, which occurs before Diff. We don't need to do anything special for Diff now.
The previous fix works on all resources... except our s3legacy bucket because it was not updated to use upstream's new tags strategy. I have changed our fork to use the new strategy. The change is here: https://github.com/pulumi/terraform-provider-aws/compare/patched-v5.9.0...patched-v5.9.0-with-modern-s3legacy-tags?expand=1
We are seeing tests fail for aws:cognito:UserPool but I know this works for aws:s3:BucketV2. These are both SDKv2 resources. Since they work differently I have ensured that both are tested.
This resource is broken upstream, so we skip the test here.
I have left skeleton code for GRPC based testing, as I often find it useful when debugging locally. I have noted that it does not run in CI, and is just there for local development.
8072478
to
fd37ad2
Compare
Fixes #2633
Fixes #2550
Relies on pulumi/pulumi-terraform-bridge#1310.
Description copied from
applyTags
's doc comment:Historically, Pulumi has struggles to handle the "tags" and "tags_all" fields correctly:
terraform-provider-aws has also struggled with implementing their desired behavior:
The Terraform lifecycle simply does not have a good way to map provider configuration onto resource values, so terraform-provider-aws is forced to work around limitations in unreliable ways. For example, terraform-provider-aws does not apply tags correctly with -refresh=false.
This gives pulumi the same limitations by default. However, unlike Terraform, Pulumi does have a clear way to insert provider configuration into resource properties: Check. By writing a custom check function that applies "default_tags" to "tags" before the Terraform provider sees any resource configuration, we can give a consistent, reliable and good experience for Pulumi users.
This strategy gives concrete benefits for Pulumi users. Diffs are applied correctly with high resolution. Consider this change:
We now can give users an accurate diff:
Changing
defaultTags
in a way that doesn't effect a resource no longer shows as an update for that resource, just for the provider:name: our-example runtime: yaml resources: p: type: pulumi:providers:aws properties: defaultTags: tags: - foo: buzz abc: def region: us-west-2 b: type: aws:s3:BucketV2 properties: tags: foo: fizz options: provider: ${p}
Of course, this also fixes the original issue; removing all
defaultTags
correctly removes them from the resource.A bonus for maintainers is that because this change takes effect at the pulumi level, we no longer rely on changing how our involved diff method works, allowing us to remove
XComputedInput
from this provider (and then from the bridge).