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

Handle provider meta better in PlanResourceChange #1826

Merged
merged 5 commits into from
Apr 9, 2024

Conversation

VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented Apr 2, 2024

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.

Copy link

codecov bot commented Apr 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.52%. Comparing base (c4bfb05) to head (9bf7d16).

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.
📢 Have feedback on the report? Share it here.

@iwahbe
Copy link
Member

iwahbe commented Apr 2, 2024

I don't really understand this change. I think a test of would make it clearer for me.

@VenelinMartinov
Copy link
Contributor Author

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 providerMeta to always return null.

There's a regression test in https://github.com/pulumi/pulumi-terraform-bridge/pull/1825/files but it's failing due to unrelated reasons.

if err != nil {
return nil, fmt.Errorf("ProviderMeta: %w", err)
}
v := cty.NullVal(metaSchema.ImpliedType())
Copy link
Member

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.

@t0yv0
Copy link
Member

t0yv0 commented Apr 2, 2024

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.

@iwahbe
Copy link
Member

iwahbe commented Apr 9, 2024

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. p.tf.Meta() is:

// 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 ProvderMetaSchema is the schema (at least in theory) for the config that was passed into the provider (the arguments for Configure). @t0yv0 I think the problem is you have the input and output domains confused in func (p *planResourceChangeImpl) providerMeta() (*cty.Value, error).

pkg/tfshim/sdk-v2/provider2.go Outdated Show resolved Hide resolved
Co-authored-by: Ian Wahbe <ian@wahbe.com>
@VenelinMartinov VenelinMartinov enabled auto-merge (squash) April 9, 2024 12:23
@VenelinMartinov VenelinMartinov merged commit 22980d5 into master Apr 9, 2024
10 checks passed
@VenelinMartinov VenelinMartinov deleted the vvm/fix_plan_resource_change_provider_meta branch April 9, 2024 12:43
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.

ProviderMeta Issue enrolling GCP resource into PlanResourceChange
3 participants