-
Notifications
You must be signed in to change notification settings - Fork 44
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
Correctly map pkg_index
tokens
#2334
Conversation
The old error message starts with: ``` panic: runtime error: index out of range [0] with length 0 goroutine 113 [running]: main.providerInfo.MakeStandard.func3({0x10203f705?, 0x1?}, {0x0?, 0x1400074dd40?}) /home/runner/work/pulumi-terraform-provider/pulumi-terraform-provider/bridge/pkg/tfbridge/tokens/tokens.go:51 +0x15c github.com/pulumi/pulumi-terraform-bridge/v3/pkg/tfbridge/tokens.KnownModules.knownResource.func1({0x10203f705?, 0x5?}, {0x0?, 0x5?}, 0x1400074dd40, {0x0?, 0x0?}) ``` The new message looks like this: ``` error: parameterize: rpc error: code = Unknown desc = 1 error occurred: * resources: 1 error occurred: * "materialize_index": empty name for module "index" ```
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2334 +/- ##
=======================================
Coverage 57.00% 57.00%
=======================================
Files 366 366
Lines 50096 50108 +12
=======================================
+ Hits 28556 28565 +9
- Misses 19973 19975 +2
- Partials 1567 1568 +1 ☔ View full report in Codecov by Sentry. |
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.
I trust your familiarity with the naming code to get this correct, but would feel better if there were more testcases checked for the exact strategy used by the dynamic provider.
err := prov.ComputeTokens(tokens.SingleModule( | ||
prov.GetResourcePrefix(), "index", tokens.MakeStandard(p.Name()))) |
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.
I can see how this fixes this specific error, and I can see that specific case tested in the new test, but I'm not sure I follow why we don't need the "_" for other resources?
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.
This is basically an instance of "we were calling tokens.SingleModule
wrong. It already knows to split on "_"
.
Name: "foo", | ||
P: (&schema.Provider{ | ||
ResourcesMap: schema.ResourceMap{ | ||
"foo_index": nil, |
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.
should we be testing some other names against this strategy eg. maybe the list fromt he previous test:
"foo_fizz_buzz": {Tok: "foo:index:FizzBuzz"},
"foo_bar_hello_world": {Tok: "foo:index:BarHelloPulumi"},
"foo_bar": {Tok: "foo:index:Bar"},
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.
This strategy is pretty well tested in:
func TestTokensSingleModule(t *testing.T) { |
fe74f59
to
610224f
Compare
This PR has been shipped in release v3.90.0. |
This PR improves the error message when it fails to map tokens (first commit), then removes the
"_"
appended to the end of the prefix so tokens that are of the form<pkg>_index
can be correctly mapped.Fixes pulumi/pulumi-terraform-provider#13