- Notifications
You must be signed in to change notification settings - Fork928
fix: Add command to reconnecting PTY#1860
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thisfixes#1708 and opens the door for PTYs to executenon-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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 inhandleReconnectingPTY?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 addingsome 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
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.
Fair enough
Thisfixes#1708 and opens the door for PTYs to executenon-shell commands!
Thisfixes#1708 and opens the door for PTYs to execute
non-shell commands!