Skip to content
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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: emissary detects tty and wraps command in pseudo terminal. Fixes #9179 #10039

Merged
merged 10 commits into from
Nov 30, 2022
Merged

fix: emissary detects tty and wraps command in pseudo terminal. Fixes #9179 #10039

merged 10 commits into from
Nov 30, 2022

Conversation

danajp
Copy link
Contributor

@danajp danajp commented Nov 15, 2022 •
edited
Loading

Fixes #9179

Please do not open a pull request until you have checked ALL of these:

  • Create the PR as draft .
  • Run make pre-commit -B to fix codegen and lint problems.
  • Sign-off your commits (otherwise the DCO check will fail).
  • Use a conventional commit message (otherwise the commit message check will fail).
  • "Fixes #" is in both the PR title (for release notes) and this description (to automatically link and close the issue).
  • Add unit or e2e tests. Say how you tested your changes. If you changed the UI, attach screenshots.
  • Github checks are green.
  • Once required tests have passed, mark your PR "Ready for review".

If changes were requested, and you've made them, dismiss the review to get it reviewed again.

@danajp danajp force-pushed the emissary-tty-support branch 4 times, most recently from c3b27e2 to 58d28a1 Compare November 15, 2022 20:02
@danajp
Copy link
Contributor Author

danajp commented Nov 15, 2022

I'd also want to backport these changes to v3.3 so we can migrate our workflows that use tty: true to emissary before upgrading to v3.4.

@danajp danajp force-pushed the emissary-tty-support branch 3 times, most recently from 8315a07 to a2bd54d Compare November 16, 2022 02:35
@danajp
Copy link
Contributor Author

danajp commented Nov 16, 2022

I'm gonna mark this ready for review. I think the e2e failures are unrelated to my changes (they didn't start until I rebased onto master).

@danajp danajp marked this pull request as ready for review November 16, 2022 13:36
tczhao
tczhao reviewed Nov 18, 2022
View reviewed changes
Copy link
Member

@tczhao tczhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi @danajp , could you push an empty commit to retrigger the tests

@danajp danajp force-pushed the emissary-tty-support branch 3 times, most recently from 4590bfc to 83e108f Compare November 18, 2022 14:14
@danajp
Copy link
Contributor Author

danajp commented Nov 18, 2022

@tczhao All green now. Pushed a couple fixes for e2e tests that were sometimes failing.

tczhao
tczhao approved these changes Nov 21, 2022
View reviewed changes
Copy link
Member

@tczhao tczhao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@danajp danajp force-pushed the emissary-tty-support branch 4 times, most recently from cfbbb76 to 3e0c1f0 Compare November 21, 2022 15:13
alexec
alexec reviewed Nov 21, 2022
View reviewed changes
go.mod
@@ -116,6 +116,7 @@ require (
github.com/cespare/xxhash/v2 v2.1.2 // indirect
github.com/chrismellard/docker-credential-acr-env v0.0.0-20220119192733-fe33c00cee21 // indirect
github.com/cpuguy83/go-md2man/v2 v2.0.2 // indirect
github.com/creack/pty v1.1.18
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MIT=OK

@alexec alexec changed the title fix: emissary detects tty and wraps command in pseudo terminal Fixes #9179 fix: emissary detects tty and wraps command in pseudo terminal. Fixes #9179 Nov 21, 2022
alexec
alexec reviewed Nov 22, 2022
View reviewed changes
Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

creack/pty#158

test by trying to compile with “GOS=windows” (or whatever it is)

@danajp
Copy link
Contributor Author

danajp commented Nov 22, 2022

OK yeah, this does not compile for windows

❯ GOOS=windows go build ./cmd/...
# github.com/argoproj/argo-workflows/v3/cmd/argoexec/commands
cmd/argoexec/commands/emissary.go:260:37: undefined: syscall.SIGWINCH
cmd/argoexec/commands/emissary.go:269:25: undefined: syscall.SIGWINCH

And digging a little further, create/pty does not support windows at all.

I'm gonna do some refactoring such that the tty support only applies to linux. @alexec should we raise an error on unsupported platforms or should we just run the process without a tty? I think my preference is for the latter.

@danajp danajp force-pushed the emissary-tty-support branch from 3e0c1f0 to 9c63e61 Compare November 22, 2022 19:45
@alexec
Copy link
Contributor

alexec commented Nov 24, 2022

Just run without tty and print a warning

@danajp danajp force-pushed the emissary-tty-support branch 2 times, most recently from 3501b30 to 64a3930 Compare November 28, 2022 15:22
@danajp
Copy link
Contributor Author

danajp commented Nov 28, 2022

I think this is ready

@danajp danajp force-pushed the emissary-tty-support branch from 73a08c6 to ac10e6a Compare November 30, 2022 15:56
@alexec alexec self-assigned this Nov 30, 2022
@alexec
Copy link
Contributor

alexec commented Nov 30, 2022

This PR is nearly ready to merge, you should sync with master and revert the e2e test that was broken.

@danajp danajp force-pushed the emissary-tty-support branch from ac10e6a to ddca9cb Compare November 30, 2022 16:23
@danajp danajp force-pushed the emissary-tty-support branch from 15a0d9c to ad6b2fa Compare November 30, 2022 18:15
@danajp danajp force-pushed the emissary-tty-support branch 2 times, most recently from e81e85b to 0834903 Compare November 30, 2022 20:11
@danajp
fix: emissary detects tty and wraps command in pseudo terminal
398a3b7 
Signed-off-by: Dana Pieluszczak <[email protected]>
@danajp
fix: Ensure bucket to be created by test does not exist
2ffa13e 
Signed-off-by: Dana Pieluszczak <[email protected]>
@danajp
fix: Pause tests should wait for a running pod
7d8130d 
Signed-off-by: Dana Pieluszczak <[email protected]>
@danajp
fix: Extract logic for starting command into os-specific package
6a24078 
Signed-off-by: Dana Pieluszczak <[email protected]>
@danajp
fix: Warn if running in TTY on windows
7d76120 
Signed-off-by: Dana Pieluszczak <[email protected]>
@danajp
fix: Only pass stdin,stdout,stderr in Cmd struct
b7d60e8 
Signed-off-by: Dana Pieluszczak <[email protected]>
@danajp
fix: Revert unrelated e2e test fix
31861cd 
Signed-off-by: Dana Pieluszczak <[email protected]>
@danajp
fix: Default cmd.Stdin to os.Stdin if nil
62aa5de 
Signed-off-by: Dana Pieluszczak <[email protected]>
@danajp
fix: Remove todo
fd49065 
Signed-off-by: Dana Pieluszczak <[email protected]>
@danajp danajp force-pushed the emissary-tty-support branch from 9a4c638 to fd49065 Compare November 30, 2022 20:52
@danajp
fix: Appease linter
0b2889b 
Signed-off-by: Dana Pieluszczak <[email protected]>
@alexec alexec merged commit f9f231e into argoproj:master Nov 30, 2022
@alexec
Copy link
Contributor

alexec commented Nov 30, 2022

LGTM!

@tooptoop4 tooptoop4 mentioned this pull request Feb 12, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcmuller jcmuller jcmuller approved these changes

@tczhao tczhao tczhao approved these changes

@alexec alexec alexec approved these changes

Assignees

@alexec alexec

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Specifying tty and stdin as true but still not getting a tty in the container
4 participants
@danajp @alexec @jcmuller @tczhao