- Notifications
You must be signed in to change notification settings - Fork913
feat(cli): add experimental rpty command#16700
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
0b27c98 toc689934Compare601dd8e toa9ff045Compare| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
review: Decided to move these to have a commonexp_ prefix. Also considered moving them to their own package but that would have been more involved.
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.
nothing blocking, I suppose
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
:)
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 had to actually try it out and itdoes work! Except you have to enter raw JSON objects to write to the terminal...
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 is very convenient for debugging purposes. Leave the websocat hint please!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| require.ErrorContains(t, err, "not found") | ||
| }) | ||
| t.Run("Container", func(t *testing.T) { |
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.
Should we consider a test case when container/agent dies?
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.
Not a bad idea! I tested it out on dogfood and it works as expected, but good to codify.
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 is a really cool command to have, thanks for adding it! A few nits inline.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
No, just search by labels currently.
Uh oh!
There was an error while loading.Please reload this page.
| ContainerUser: args.ContainerUser, | ||
| Width: termWidth, | ||
| Height: termHeight, | ||
| }) |
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.
Since this goes via coderd; do we need the "direct connection" check at all?
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 discussed this with Ben and we agreed to keep it even though there's a trivial workaround. We can adjust based on customer feedback.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| }); err != nil { | ||
| return | ||
| } | ||
| } |
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.
Debounce would be "nice", but since this is an experimental command, I'm not pushing for it.
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.
What do you specifically mean by 'debounce' here? Multiple keypresses?
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..isn't it overengineering in this case?
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 don't think the web terminal doesany debouncing, so this is potentially a later improvement in both cases.
Uh oh!
There was an error while loading.Please reload this page.
c5a265f intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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 .