Skip to content

Support embedded Runme servers and the connect protocol#767

Closed
jlewi wants to merge 7 commits intorunmedev:mainfrom
jlewi:jlewi/grpcweb
Closed

Support embedded Runme servers and the connect protocol#767
jlewi wants to merge 7 commits intorunmedev:mainfrom
jlewi:jlewi/grpcweb

Conversation

@jlewi
Copy link
Copy Markdown
Contributor

@jlewi jlewi commented Mar 19, 2025

I'd like to be able to link Runme into an existing CLI. This way users don't have to download a new binary in order to be able to run runme as a server; I can just distribute as part of my existing binary server.

The blocker is that most of the Runme libraries are hidden inside the internal package which prevents other packages from importing them as a library.

This creates a thin non-hidden wrapper for the stuff I need. Arguably, refactoring the code out of the internal package is a better solution but that seems like it could be a bit of a lift.

This PR also updates buf.gen.yaml so we can generate connectrpc golibs. It turns out that trying to link golibraries from the buf registry and simulatenously link Runme as a library leads to name packages.

@sourishkrout I'm not really blocked on this because I can just use a rewrite declaration in go.mod to pull in my fork. So no rush.

jlewi added 6 commits March 17, 2025 13:27
Signed-off-by: Jeremy Lewi <jeremy@lewi.us>
Signed-off-by: Jeremy Lewi <jeremy@lewi.us>
Signed-off-by: Jeremy Lewi <jeremy@lewi.us>
Signed-off-by: Jeremy Lewi <jeremy@lewi.us>
Signed-off-by: Jeremy Lewi <jeremy@lewi.us>
Signed-off-by: Jeremy Lewi <jeremy@lewi.us>
@jlewi
Copy link
Copy Markdown
Contributor Author

jlewi commented Mar 19, 2025

There are some lint failures but I'm going to punt on resolving them until we figure out what to do about this PR.
I

@sourishkrout
Copy link
Copy Markdown
Contributor

There are some lint failures but I'm going to punt on resolving them until we figure out what to do about this PR. I

That's fair. As long as this isn't blocking you.

I have yet to take a closer look and make up my mind on how to support linking.

@sourishkrout
Copy link
Copy Markdown
Contributor

This PR also updates buf.gen.yaml so we can generate connectrpc golibs. It turns out that trying to link golibraries from the buf registry and simulatenously link Runme as a library leads to name packages.

🤔 connectrpc seems superfluous to me since protobuf-es is just as good and doesn't require a facade layer. Both protobuf-es and connectrpc are both blocked by a lack of a WebTransport grpc-transport (client for bidi) in the browser. I trust you have a plan.

@jlewi
Copy link
Copy Markdown
Contributor Author

jlewi commented Mar 20, 2025

So originally I needed connectrpc because I want to use the Connect GoLang Server(jlewi/cloud-assistant#4). Which requires generating the connectrpc golang generated libraries. I was then connecting to it using grpc-web as the transport. Originally I was only planning on using server side streaming as that is what is supported.

If we are using websockets, then the Runme service no longer needs to be exposed as a GRPC/Connect service in my client in which case generating golang connectrpc clients should no longer be necessary. So we could drop that part.

I think I still want to be able to link in Runme to an existing binary which requires finding someway to relax the internal visibility restriction.

I'm fine waiting to ensure there is sufficient value in linking in Runme before modifying code on main to support it. I'm fine with closing this PR for now or marking it as draft.

@sourishkrout
Copy link
Copy Markdown
Contributor

So originally I needed connectrpc because I want to use the Connect GoLang Server(jlewi/cloud-assistant#4). Which requires generating the connectrpc golang generated libraries. I was then connecting to it using grpc-web as the transport. Originally I was only planning on using server side streaming as that is what is supported.

If we are using websockets, then the Runme service no longer needs to be exposed as a GRPC/Connect service in my client in which case generating golang connectrpc clients should no longer be necessary. So we could drop that part.

I think I still want to be able to link in Runme to an existing binary which requires finding someway to relax the internal visibility restriction.

I'm fine waiting to ensure there is sufficient value in linking in Runme before modifying code on main to support it. I'm fine with closing this PR for now or marking it as draft.

Aye on both. I'll look into the best way to remove the internal obstacle.

Moving the PR into draft for now.

@sourishkrout sourishkrout marked this pull request as draft March 20, 2025 23:53
@jlewi
Copy link
Copy Markdown
Contributor Author

jlewi commented Mar 21, 2025

One thing I wasn't aware of was the envCollector.

type envCollector interface {

It looks like this is always set currently by the CommandFactory
return NewEnvCollectorFactory().Build()

This ends up having Runme invoke itself at the (start? and/or end?) of every command in order to dump/collect the environment
I think it ends up invoking this command

cmd := cobra.Command{

So from the perspective of linking in Runme into an existing binary we would potentially need to link in that functionality as well or make it possible to disable it.

@sourishkrout
Copy link
Copy Markdown
Contributor

So from the perspective of linking in Runme into an existing binary we would potentially need to link in that functionality as well or make it possible to disable it.

Runme's stateful execution (making the ENV sticky) won't work without the EnvCollector as part of Snapshotting. Don't think you want to disable it. However, it should be easy to replicate the <binary> env dump ... cmd which mimics more or less what Unix's env does. I can't remember if we implemented it because it wasn't available in all runtimes, e.g. container images, or if there were difference Linux vs Darwin vs Windows etc. Perhaps both.

My suggestion would be to only replicate this command and ignore the rest of the env store ... cmd group. All you need is: https://github.com/runmedev/runme/blob/main/internal/cmd/environment.go#L296-L320

@jlewi
Copy link
Copy Markdown
Contributor Author

jlewi commented Mar 21, 2025

However, it should be easy to replicate the env dump ... cmd which mimics more or less what Unix's env does
Seems reasonable.

Note to self: We also need to figure out how to properly deal with it being different subcommands when linking Runme into unrelated binaries.

@sourishkrout now that we are using websockets; I don't think we need ExecuteOneshot (#764) . Want me to send you a revert PR?

@sourishkrout
Copy link
Copy Markdown
Contributor

@sourishkrout now that we are using websockets; I don't think we need ExecuteOneshot (#764) . Want me to send you a revert PR?

No need, thank you. I'll remove it.

@jlewi
Copy link
Copy Markdown
Contributor Author

jlewi commented Mar 31, 2025

I think this is superceded by the branch
https://github.com/runmedev/runme/tree/seb/env-collector

@jlewi jlewi closed this Mar 31, 2025
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.

2 participants