feat(cli): add experimental rpty command #16700
Conversation
0b27c98 to
c689934
Compare
601dd8e to
a9ff045
Compare
| // This test will time out if the user has commit signing enabled. | ||
| if _, gpgTTYFound := os.LookupEnv("GPG_TTY"); gpgTTYFound { | ||
| t.Skip("GPG_TTY is set, skipping test to avoid hanging") | ||
| } |
There was a problem hiding this comment.
review: decided to add this drive-by since running the tests locally was hanging for me
cli/exp_errors.go
Outdated
There was a problem hiding this comment.
review: Decided to move these to have a common exp_ prefix. Also considered moving them to their own package but that would have been more involved.
| cmd := &serpent.Command{ | ||
| Handler: func(inv *serpent.Invocation) error { | ||
| if r.disableDirect { | ||
| return xerrors.New("direct connections are disabled, but you can try websocat ;-)") |
There was a problem hiding this comment.
I had to actually try it out and it does work! Except you have to enter raw JSON objects to write to the terminal...
There was a problem hiding this comment.
This is very convenient for debugging purposes. Leave the websocat hint please!
| require.ErrorContains(t, err, "not found") | ||
| }) | ||
|
|
||
| t.Run("Container", func(t *testing.T) { |
There was a problem hiding this comment.
Should we consider a test case when container/agent dies?
There was a problem hiding this comment.
Not a bad idea! I tested it out on dogfood and it works as expected, but good to codify.
| for _, ct := range cts.Containers { | ||
| if ct.FriendlyName == args.Container || ct.ID == args.Container { | ||
| ctID = ct.ID | ||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
Does the agent API support searching based on name/id? If not, might be nice to have the logic there (not a blocker for this PR though).
There was a problem hiding this comment.
No, just search by labels currently.
| ContainerUser: args.ContainerUser, | ||
| Width: termWidth, | ||
| Height: termHeight, | ||
| }) |
There was a problem hiding this comment.
Since this goes via coderd; do we need the "direct connection" check at all?
There was a problem hiding this comment.
I discussed this with Ben and we agreed to keep it even though there's a trivial workaround. We can adjust based on customer feedback.
| }); err != nil { | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
Debounce would be "nice", but since this is an experimental command, I'm not pushing for it.
There was a problem hiding this comment.
What do you specifically mean by 'debounce' here? Multiple keypresses?
There was a problem hiding this comment.
Hmm..isn't it overengineering in this case?
There was a problem hiding this comment.
I don't think the web terminal does any debouncing, so this is potentially a later improvement in both cases.
Relates to #16419
Builds upon #16638 and adds a command
exp rptythat allows you to open a ReconnectingPTY session to an agent.This ultimately allows us to add an integration-style CLI test to verify the functionality added in #16638 .