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

Do not map resources/datasources specified in IgnoreMappings #1840

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

iwahbe
Copy link
Member

@iwahbe iwahbe commented Apr 4, 2024

Fixes #1838

Previously, we applied (in bridge) automatic mapping to resources/datasources that didn't have an associated Tok field, even if they were included in IgnoreMappings. For providers that vendored resources from multiple prefixes, this caused a panic.

The solution is to not provide a mapping for resources that are specified in IngoreMappings. This expands the scope of IngoreMappings, but does so in a way that I feel is more intuitive then it is now. I have adjusted the comment on IgnoreMappings to reflect it's new scope.

Fixes #1838

Previously, we applied (in bridge) automatic mapping to resources/datasources that didn't
have an associated `Tok` field, even if they were included in `IgnoreMappings`. For
providers that vendored resources from multiple prefixes, this caused a panic.

The solution is to not provide a mapping for resources that are specified in
`IngoreMappings`. This expands the scope of `IngoreMappings`, but does so in a way that I
feel is more intuitive then it is now. I have adjusted the comment on `IgnoreMappings` to
reflect it's new scope.
@iwahbe iwahbe requested review from t0yv0 and a team April 4, 2024 10:49
@iwahbe iwahbe self-assigned this Apr 4, 2024
Copy link

codecov bot commented Apr 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 60.28%. Comparing base (9257734) to head (249ae7f).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1840      +/-   ##
==========================================
- Coverage   60.75%   60.28%   -0.48%     
==========================================
  Files         303      310       +7     
  Lines       42377    42773     +396     
==========================================
+ Hits        25747    25785      +38     
- Misses      15157    15517     +360     
+ Partials     1473     1471       -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.

looks good, thanks for the tests.

Is it possible to surprise any provider authors with the new behaviour? Is it not possible now for some resource to go missing silently?

// Fetch a list of all resource types handled by this provider and make a map.
p.resources = make(map[tokens.Type]Resource)
p.tf.ResourcesMap().Range(func(name string, res shim.Resource) bool {
schema, ok := p.info.Resources[name]
Copy link
Contributor

Choose a reason for hiding this comment

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

We checked for p.info.Resources == nil before - are you allowed to call [name] on a nil map?

Copy link
Member Author

Choose a reason for hiding this comment

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

You are allowed to treat a nil map as indexable, if it is an rvalue:

x := mayBeNil[name] // This is ok, x is the zero value

mayBeNil[name] = x // This will panic

@iwahbe
Copy link
Member Author

iwahbe commented Apr 4, 2024

Is it possible to surprise any provider authors with the new behaviour? Is it not possible now for some resource to go missing silently?

It is possible, but highly unlikely. This PR does change the runtime behavior of the provider (removing resources), but it doesn't change the tfgen time behavior.

For this to break a provider in use, I believe that the provider's consumers would need to calling RegisterResource directly (instead of using a SDK).

@iwahbe iwahbe merged commit 3b6afd5 into master Apr 4, 2024
10 checks passed
@iwahbe iwahbe deleted the iwahbe/1838/panic-from-unmapped-resources branch April 4, 2024 11:15
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.

Community provider panics at runtime with unmapped resources
2 participants