Fix ImportBasePath for dynamic bridged providers #2705
Conversation
|
This looks like itll fix the issue that I was running into |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2705 +/- ##
==========================================
- Coverage 69.51% 69.50% -0.02%
==========================================
Files 301 301
Lines 38637 38637
==========================================
- Hits 26860 26855 -5
- Misses 10256 10261 +5
Partials 1521 1521 ☔ View full report in Codecov by Sentry. |
|
We don't do this for Any TF Provider code. Not having the double nesting worked in the past. Has something changed? |
Fraser's answer will almost certainly be more pointed but this has to do with the way we generate sdks locally in pulumi and the go code is expected to be nested due to the possiblity of multiple parameterizations and is how conformance tests (and the generated sdks are expected to) behave. It also avoids name overlap |
Normal TF providers use a different sdk-gen to dynamic TF providers. When we generate a normal TF provider codegen doesn't output any go.mod and leaves it up to provider authors to add that to their repo wherever they want. For parameterised packages we do output a go.mod and we output it at a specific place just above the provider package, so we end up with: If you set the ImportBasePath to We're not doing that. It doesn't even match existing sdk layouts.
We had a bug where we were generating two go.mods before for parameterised packages, one at the root as expected and another inside the package directory. The one in the package directory could be used with the import path as given, although it didn't match up if actually pushed to GitHub. |
We should do that. It's marginally better for users and the double-nesting seems to have no benefit beyond path dependency. I doubt we are going to change it now.
It shouldn't matter. Codegen should just trust the import path. |
There was a problem hiding this comment.
This change will need to come with docs updates:
If ImportBasePath has correctness requirements, they need to be documented. Ideally, pulumi package use should warn (or fail) for an invalid ImportBasePath.
It does mostly, but when we're trying to infer the module path just from the import path we don't have enough info unless we can assume things are laid out exactly as we expect (which means double nesting) or if the schema gives us the module path (which tfbridge doesn't), but then...
I think we could if we had the module path from the schema as well, but this all requires codegen fixes to change the sdk layout based on those two fields.
That's probably the better fix here.
Sure, I can add that. |
See pulumi/pulumi-terraform-bridge#2705 That changed the ImportBasePath used for Go packages to match up what sdk-gen currently writes in terms of folder structure. This updates the docs so the import statments match.
|
This PR has been shipped in release v3.97.1. |
See pulumi/pulumi-terraform-bridge#2705 That changed the ImportBasePath used for Go packages to match up what sdk-gen currently writes in terms of folder structure. This updates the docs so the import statments match.
The current code generates an import base path like
github.com/pulumi/pulumi-terraform-provider/sdks/something/v3. This is a module path (ends with a version number) not a package import path, we always nest our packages one level below the module.The correct import base path should be
github.com/pulumi/pulumi-terraform-provider/sdks/something/v3/something.