-
Notifications
You must be signed in to change notification settings - Fork 41
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
Handle provider meta better in PlanResourceChange #1826
Handle provider meta better in PlanResourceChange #1826
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1826 +/- ##
==========================================
- Coverage 61.04% 60.52% -0.53%
==========================================
Files 303 310 +7
Lines 42444 42810 +366
==========================================
Hits 25911 25911
- Misses 15065 15431 +366
Partials 1468 1468 ☔ View full report in Codecov by Sentry. |
I don't really understand this change. I think a test of would make it clearer for me. |
This code would panic on the GCP provider. It is not used in AWS and nothing else uses the PlanResourceChange stuff. I'd like to use PlanResourceChange in GCP. To do that, I've "fixed" the There's a regression test in https://github.com/pulumi/pulumi-terraform-bridge/pull/1825/files but it's failing due to unrelated reasons. |
pkg/tfshim/sdk-v2/provider2.go
Outdated
if err != nil { | ||
return nil, fmt.Errorf("ProviderMeta: %w", err) | ||
} | ||
v := cty.NullVal(metaSchema.ImpliedType()) |
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.
Add a test please. Otherwise LGTM. Some of your PR comment may be worth repeating in source.
I think p.tf.Meta() and ProvderMetaSchema are just different things right? I remember bumping into this before, it keeps being difficult to remember because they're both named Meta. I must have crossed the wrong wires when writing providerv2 code. |
I'm pretty sure that they are unrelated. // Meta returns the metadata associated with this provider that was
// returned by the Configure call. It will be nil until Configure is called.
func (p *Provider) Meta() interface{} {
return p.meta
} I'm pretty sure that it is provider local and never needs to be serialized. I think |
Co-authored-by: Ian Wahbe <ian@wahbe.com>
fixes #1822 and unlocks pulumi/pulumi-gcp#1874
ProviderMeta
seems to be an old experimental feature of TF which has not picked up.AWS does not use it and GCP uses it only for one thing - setting UserAgent strings: modular-magician/terraform-provider-google-beta@6cf6c51
The meta we receive does not match the schema provided and does not contain the module_name property. We should set it to null and potentially revisit if this causes issues. I've tested it for the GCP resource in #1825 and works fine.
I've opened #1827 as a follow-up to test with one of the resources which actually use the ProviderMeta.