feat: add resource urn to context in Check call #1877
Conversation
This PR adds the resource URN to the context in the `Check` method, along with methods to set and get the URN from context. The motivation for this change was driven by trying to implement the suggested fix in [this comment](pulumi/pulumi-aws#3788 (comment)) in the aws provider. We want to be able to keep some global state of which resources have been seen by `Check`, but realized that we don't actually get any identifying information in the `PreCheckCallback` function. An alternative approach would be to update the `PreCheckCallback` signature to also contain the URN, but that would be a breaking change. re pulumi/pulumi-aws#3788
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1877 +/- ##
==========================================
+ Coverage 59.99% 60.01% +0.01%
==========================================
Files 327 327
Lines 43976 43986 +10
==========================================
+ Hits 26382 26396 +14
+ Misses 16102 16098 -4
Partials 1492 1492 ☔ View full report in Codecov by Sentry. |
pkg/tfbridge/provider.go
Outdated
| @@ -726,6 +726,7 @@ func (p *Provider) Configure(ctx context.Context, | |||
| func (p *Provider) Check(ctx context.Context, req *pulumirpc.CheckRequest) (*pulumirpc.CheckResponse, error) { | |||
| ctx = p.loggingContext(ctx, resource.URN(req.GetUrn())) | |||
| urn := resource.URN(req.GetUrn()) | |||
| ctx = WithUrn(ctx, urn) | |||
There was a problem hiding this comment.
I'm amenable to this but the question becomes why only in Check? Perhaps you could retrofit p.loggingContext to do this; it's already installed in all the methods pretty much.
Co-authored-by: Ian Wahbe <ian@wahbe.com>
|
An alternative to consider is to add a |
|
Check does not return planed state in bridged providers, alas. |
There was a problem hiding this comment.
As implemented, I don't think this will work for PF based providers. If I'm wrong and it does, then we need to add a quick test to lock in that behavior. If it doesn't, we will need to add support (and a quick test to lock in behavior).
The equivalent method to loggingContext in PF land is initLogging:
pulumi-terraform-bridge/pf/tfbridge/logging.go
Lines 25 to 28 in de2933b
Done! |
This PR adds the resource URN to the context in the
Checkmethod, along with methods to set and get the URN from context.The motivation for this change was driven by trying to implement the suggested fix in this
comment in the aws provider. We want to be able to keep some global state of which resources have been seen by
Check, but realized that we don't actually get any identifying information in thePreCheckCallbackfunction.An alternative approach would be to update the
PreCheckCallbacksignature to also contain the URN, but that would be a breaking change.re pulumi/pulumi-aws#3788