Reimplement getRequiredPlugins for go sdk #4297
Conversation
EvanBoyle
requested review from
pgavlin and
lukehoban
and removed request for
pgavlin
sdk/go/pulumi-language-go/main.go
Outdated
| func getPluginName(modName string) string { | ||
| // github.com/pulumi/pulumi-aws/sdk/... => aws | ||
| pluginPart := strings.Split(modName, "/")[2] | ||
| return strings.SplitN(pluginPart, "-", 2)[1] |
There was a problem hiding this comment.
Both of these indexes could panic right? We should guard them and skip cleanly?
There was a problem hiding this comment.
Oh - I see that the caller ensures they can't.
There was a problem hiding this comment.
Correct. I refactored this to make the relationship clear in case we change this code in the future.
| } | ||
|
|
||
| // don't wire up stderr so non-module users don't see error output from list | ||
| cmd := exec.Command(gobin, "list", "-m", "-json", "all") |
There was a problem hiding this comment.
I guess there is no built-in library that can be used to do this? Looks like the implementation of this command is all internal unfortunately.
There was a problem hiding this comment.
Yup, tried this initially and learned about the internal restriction.
sdk/go/pulumi-language-go/main.go
Outdated
| if strings.HasPrefix(m.Path, "github.com/pulumi/pulumi-") { | ||
| plugin := &pulumirpc.PluginDependency{ | ||
| Name: getPluginName(m.Path), | ||
| Version: m.Version, |
There was a problem hiding this comment.
These versions can be things like "v0.0.0-20200317142112-1b76d66859c6". Do we need to do any filtering here to not return these back? Or do we rely on the other side of this interface being okay with weird versions it doesn't understand?
There was a problem hiding this comment.
Good point. I updated this to handle non-tag commits (we normalize the patch to 0 to be compatible). The only way we can get something like v0.0.0 is when the module is new and has no associated tags, which should never happen now that we've upgraded to the new module structure. I suppose we could filter out 0.0.0 if we want to be extra careful. I'm open to suggestions here.
| // GetRequiredPlugins computes the complete set of anticipated plugins required by a program. | ||
| // We're lenient here as this relies on the `go list` command and the use of modules. | ||
| // If the consumer insists on using some other form of dependency management tool like | ||
| // dep or glide, the list command fails with "go list -m: not using modules" | ||
| func (host *goLanguageHost) GetRequiredPlugins(ctx context.Context, |
There was a problem hiding this comment.
Can we add any test coverage of this?
There was a problem hiding this comment.
I refactored to test the code to test the filtering and conversion of the module result from go list into a plugin description.
This seems sufficient to me. Any further validation would require an integration test with a dependency on pulumi-aws.
sdk/go/pulumi-language-go/main.go
Outdated
| // downgrade to zero for the patch | ||
| // pinning to a specific commit can result in a minor version that doesn't exist | ||
| // v1.29.1-0.20200403140640-efb5e2a48a86 (first commit after 1.29.0 release) | ||
| version := fmt.Sprintf("v%v.%v.%v", v.Major, v.Minor, 0) |
There was a problem hiding this comment.
In the case that a precise version was specified - this seems wrong.
Also in general - I’m not positive this won’t work - for Kubernetes for example I think dev builds are prerelease for major version bumps? And is dependent on some very specific things about how we currently version dev tags. Anything more reliable we could do here?
Perhaps no and this is still the best we can do heuristically and is worth it?
There was a problem hiding this comment.
Good point. I'll lift some of the pseduo version parsing code out of internal to make this more robust.
|
@lukehoban Please take another look. I took some code out of internal that detects pseudo versions so we can special case their handling. This allows us to leave patch versions alone in the normal case where our version matches a tag. This also allows us to handle beta/dev tags. I added more unit test cases to document the behavior of more of these corner cases. I manually tested the |
Co-Authored-By: Luke Hoban <luke@pulumi.com>
This uses
go list -m -json all. We must use -mmodulemode as this is the only way to get the version info as far as I can tell. This means that this approach only works for users using modules, not glide, dep, etc. This is the direction the community is going, and the direction we've moved our docs/examples so this should be fine. But as a result, I've made the implementation lenient. It doesn't fail if we run into errors along the way, just logs some messages saying we failed to acquire plugins. This is strictly better than what we have today, and we can always revisit this decision in the future as the ecosystem evolves.Fixes #1549
Fixes #4211