-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Revise host mode. #5317
Revise host mode. #5317
Conversation
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.
LGTM. Would be good for @EvanBoyle to review as well.
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 looks great. Substantially cleaner and easier to reason about than the original approach. Thanks for taking the time to dig into this! Just a few notes/questions.
return nil, "", err | ||
} | ||
if clientAddress != "" { | ||
proj.Runtime = workspace.NewProjectRuntimeInfo("client", map[string]interface{}{ |
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 believe we rely on language info being present here for other parts of the system. I'm aware that we used this approach in the host command but I think that was an oversight.
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'm going to leave this as-is for now and address it in a follow-up change. I'll file an issue to track.
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.
The current implementation of host mode uses a `pulumi host` command and an ad-hoc communication protocol between the engine and client to connect a language host after the host has begun listening. The most significant disadvantages of this approach are the communication protocol (which currently requires the use of stdout), the host-specific command, and the difficulty of accommodating the typical program-bound lifetime for an update. These changes reimplement host mode by adding engine support for connecting to an existing language runtime service rather than launching a plugin. This capability is provided via an engine-specific language runtime, `client`, which accepts the address of the existing languge runtime service as a runtime option. The CLI exposes this runtime via the `--client` flag to the `up` and `preview` commands, which similarly accepts the address of an existing language runtime service as an argument. These changes also adjust the automation API to consume the new host mode implementation.
Co-authored-by: Justin Van Patten <jvp@justinvp.com>
a5a3925
to
5dfdab1
Compare
The current implementation of host mode uses a
pulumi host
command andan ad-hoc communication protocol between the engine and client to
connect a language host after the host has begun listening. The most
significant disadvantages of this approach are the communication
protocol (which currently requires the use of stdout), the host-specific
command, and the difficulty of accommodating the typical program-bound
lifetime for an update.
These changes reimplement host mode by adding engine support for
connecting to an existing language runtime service rather than launching
a plugin. This capability is provided via an engine-specific language
runtime,
client
, which accepts the address of the existing langugeruntime service as a runtime option. The CLI exposes this runtime via
the
--client
flag to theup
andpreview
commands, which similarlyaccepts the address of an existing language runtime service as an
argument. These changes also adjust the automation API to consume the
new host mode implementation.