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

Infer Map[string] when the Map's element type is unspecified #2316

Merged
merged 1 commit into from
Aug 15, 2024

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Aug 15, 2024

SDKv2 providers allow specifying a map type without a backing element type:

schema.Schema{
	Type:     schema.TypeMap,
	Required: true,
}

Previously, the bridge interpreted this as map[any]. This does not agree with TF's interpretation: map[string]. This PR changes the bridge to correctly interpret map[???] as map[string].

Fixes pulumi/pulumi-nomad#389

@iwahbe iwahbe self-assigned this Aug 15, 2024
Copy link

codecov bot commented Aug 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 56.90%. Comparing base (0f1e694) to head (4184f1f).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2316      +/-   ##
==========================================
- Coverage   56.93%   56.90%   -0.03%     
==========================================
  Files         365      365              
  Lines       49957    49961       +4     
==========================================
- Hits        28443    28431      -12     
- Misses      19973    19987      +14     
- Partials     1541     1543       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@VenelinMartinov VenelinMartinov left a comment

Choose a reason for hiding this comment

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

Great find! Did you test this in nomad?

Can we also add an integration test for the panic?

@t0yv0
Copy link
Member

t0yv0 commented Aug 15, 2024

One more case missed in #1828 it seems.

@iwahbe
Copy link
Member Author

iwahbe commented Aug 15, 2024

Great find! Did you test this in nomad?

Can we also add an integration test for the panic?

I tested this in nomad, the error case becomes unrepresentable. I haven't added an integration test for the panic in nomad, and we shouldn't need to:

The bridge translation ensures that (TF) Schema{Type: Map} is translated to (Pulumi) { Type: Map, Elem: {Type: String}}. As long as that holds (and it does, with tests) then SDKs (and YAML) will prevent sending malformed inputs. If a user tries to send malformed inputs, then Type-check incoming values against the Pulumi Package Schema #1328 should let them know that what they are doing is wrong (and catch the panic).

Copy link
Contributor

@VenelinMartinov VenelinMartinov left a comment

Choose a reason for hiding this comment

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

That makes sense, you are saying the tfgen test is sufficient here. Sounds good

@iwahbe iwahbe merged commit 662459e into master Aug 15, 2024
11 checks passed
@iwahbe iwahbe deleted the iwahbe/infer-missing-map-types branch August 15, 2024 15:57
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.

Crash when using vars that are not strings
3 participants