fix: Add command to reconnecting PTY #1860
Conversation
This fixes #1708 and opens the door for PTYs to execute non-shell commands!
| // | ||
| // The command is optional and defaults to start a shell. | ||
| func (c *Conn) ReconnectingPTY(id string, height, width uint16, command string) (net.Conn, error) { | ||
| channel, err := c.CreateChannel(context.Background(), fmt.Sprintf("%s:%d:%d:%s", id, height, width, command), &peer.ChannelOptions{ |
There was a problem hiding this comment.
How about taking this opportunity to add a version indicator:
| channel, err := c.CreateChannel(context.Background(), fmt.Sprintf("%s:%d:%d:%s", id, height, width, command), &peer.ChannelOptions{ | |
| channel, err := c.CreateChannel(context.Background(), fmt.Sprintf("v1:%s:%d:%d:%s", id, height, width, command), &peer.ChannelOptions{ |
and a corresponding check in handleReconnectingPTY?
Without it, this change makes it much harder to ever add a fifth field in the future, because we wouldn't be able to reliably distinguish a 5-field label from a 4-field one whose "command" field contains a colon. And since we're passing the command directly to the shell, it seems like a situation where an incorrect parse could have bad consequences.
There was a problem hiding this comment.
Hmm seems like this surfaces the greater issue of incompatibility with agent<->coder versions. I'm nervous about versioning this because it really masks the greater problem.
Am I understanding that correctly? If so, I can create an issue for enforcing some level of compatibility between versions.
There was a problem hiding this comment.
I definitely agree that this is just the tip of a bigger issue that we don't need to worry about solving right now. I just think that adding some kind of version indicator now -- no matter how simple -- will be enough to avoid painting ourselves into a corner.
The current version can use "too many colons" as an indicator that there's a version mismatch, and at least crash rather than silently doing the wrong thing, but this patch gets rid of that possibility.
There was a problem hiding this comment.
adding some kind of version indicator now
Or more accurately, adding it before people start seriously using v2 in production. Doesn't have to be literally now, so it's not really a blocker for this PR.
This fixes #1708 and opens the door for PTYs to execute non-shell commands!
This fixes #1708 and opens the door for PTYs to execute
non-shell commands!