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

Engine and Golang support for shimless providers #10916

Merged
merged 1 commit into from Nov 17, 2022
Merged

Conversation

Frassle
Copy link
Member

@Frassle Frassle commented Oct 4, 2022

Description

This allows the pulumi-language-go plugin to start up providers directly from .go source files.

The other language providers will be extended to support this as well in time.

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have run make changelog and committed the changelog/pending/<file> documenting my change
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

@pulumi-bot
Copy link
Contributor

pulumi-bot commented Oct 4, 2022

Changelog

[uncommitted] (2022-11-14)

Features

  • [engine] Engine and Golang support for language plugins starting providers directly.
    #10916

@@ -1,5 +1,5 @@
{
"go.buildTags": "all smoke",
Copy link
Member Author

Choose a reason for hiding this comment

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

This stopped "run test" widgets working in non-smoke test files

Copy link
Member

Choose a reason for hiding this comment

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

I have a PR to fix this - I think I've just forgotten to merge it!

Copy link
Member

Choose a reason for hiding this comment

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

Queued: #10879

Copy link
Member

Choose a reason for hiding this comment

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

If that fails to merge for any reason, I'll pull the change into a separate PR.

@Frassle Frassle force-pushed the fraser/shimlessGo branch 2 times, most recently from 9893164 to aa4cd71 Compare October 4, 2022 12:22
Comment on lines +122 to +134
func MakeInstallDependenciesStreams(
server pulumirpc.LanguageRuntime_InstallDependenciesServer,
isTerminal bool) (io.Closer, io.Writer, io.Writer, error) {

return makeStreams(
func(b []byte) error {
return server.Send(&pulumirpc.InstallDependenciesResponse{Stdout: b})
},
func(b []byte) error {
return server.Send(&pulumirpc.InstallDependenciesResponse{Stderr: b})
},
isTerminal)
}
Copy link
Member

Choose a reason for hiding this comment

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

Will npm continue to render a progress bar (re: #10901) on unix-y platforms, at least? It looks like this indirection shouldn't change the behavior.

It seems like it would be a pain to write a test to verify, so I'll just ask to confirm. (Not sure what we could test, could we snoop the stream and detect if it's sending VT commands?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeh this shouldn't change the current InstallDependencies behavior, I just ended up with a slightly different proto-layout. Given we update all these things in lock-step I could probably do a sanitize pass over this to bring everything into the same system, just need to do the dance with the yaml and java repos so wouldn't want to do it just before a release day.

@@ -39,6 +39,9 @@ service LanguageRuntime {

// GetProgramDependencies returns the set of dependencies required by the program.
rpc GetProgramDependencies(GetProgramDependenciesRequest) returns (GetProgramDependenciesResponse) {}

// RunPlugin executes a plugin program and returns its result asynchronously.
rpc RunPlugin(RunPluginRequest) returns (stream RunPluginResponse) {}
Copy link
Member

Choose a reason for hiding this comment

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

Frustrating that we can't have a tuple of immediate data and a trailing stream in a single RPC. The response API will look really funny if we want to add a message type for communicating a structured failure starting the plugin outside of the context of running it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeh I really wish grpc streams were a three type message of (immediate response message, many stream messages, final terminating message), but we work with what we've got.

Copy link
Member

@AaronFriel AaronFriel 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 to me, but I'm curious about the test skip.

@@ -663,6 +663,9 @@ func testComponentProviderSchema(t *testing.T, path string) {
test := test
t.Run(test.name, func(t *testing.T) {
t.Parallel()

t.Skip("This needs to use a plugin host to deal with non-native-binary providers")
Copy link
Member

Choose a reason for hiding this comment

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

Why is this test being skipped when it previously passed?

Copy link
Member Author

Choose a reason for hiding this comment

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

So this actually only matters for Go so I'll move the skip to just that test (in tests/integration/integration_go_test.go) but it's because this method is attempting to exec the plugin binary directly and then manually wrapping the grpc machinery around it, but I've changed component_setup.sh so we don't actually build the go component anymore so there is no native binary for it to exec.

I'll just skip the go test for now, and I'll rewrite this test to use the plugin host machinery properly before the node and python components are similarly stripped down.

This allows the pulumi-language-go plugin to start up providers directly
from .go source files.

The other language providers will be extended to support this as well in
time.
Copy link
Member

@AaronFriel AaronFriel left a comment

Choose a reason for hiding this comment

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

I think I'm happy with these changes.

@Frassle
Copy link
Member Author

Frassle commented Nov 17, 2022

bors merge

@bors
Copy link
Contributor

bors bot commented Nov 17, 2022

Build succeeded:

@bors bors bot merged commit 3e98035 into master Nov 17, 2022
@bors bors bot deleted the fraser/shimlessGo branch November 17, 2022 09:41
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.

None yet

3 participants