This repository has been archived by the owner on Jun 2, 2022. It is now read-only.
Generalize console control #623
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
requested review from
MikaelSmith
force-pushed
the
fix-kubectl-exec
branch
from
07fab6f to
122dce0
Compare
|
So this handles the case when the plugin's API prompts for something, right? Then having control of the console prior to a method invocation would make sense. |
MikaelSmith
commented
Nov 26, 2019
|
Yes. |
MikaelSmith
force-pushed
the
fix-kubectl-exec
branch
from
122dce0 to
b34b32a
Compare
|
Ok, I think this works better now. |
MikaelSmith
force-pushed
the
fix-kubectl-exec
branch
2 times, most recently
from
be84dba to
7568973
Compare
ekinanp
reviewed
Nov 26, 2019
| signal.Notify(sigCh, syscall.SIGINT) | ||
| cancelCtx, cancel := context.WithCancel(ctx) | ||
| go func() { | ||
| <-sigCh |
There was a problem hiding this comment.
This won't block forever, will it? Guessing the Reset below prevents that?
There was a problem hiding this comment.
Hmm, it might. I can probably do something about that.
There was a problem hiding this comment.
Should be addressed now.
There was a problem hiding this comment.
Why not just close sigCh in a defer? You'll still need to call the cancel func either way
There was a problem hiding this comment.
Oh, didn't think of it.
MikaelSmith
force-pushed
the
fix-kubectl-exec
branch
from
7568973 to
e471da1
Compare
Sometimes a plugin needs control of the console for a prompt it doesn't make directly. Get control before calling all primitives that take a context (as a proxy for potential network activity or execs). This isn't entirely safe. If it's backgrounded and prompts for input it may not exit cleanly. Using `plugin.Prompt` guards against this issue. Fixes puppetlabs-toy-chest#297. Signed-off-by: Michael Smith <[email protected]>
MikaelSmith
force-pushed
the
fix-kubectl-exec
branch
from
e471da1 to
9219138
Compare
ekinanp
approved these changes
Nov 27, 2019
MikaelSmith
added a commit
to MikaelSmith/wash
that referenced
this pull request
Dec 2, 2019
When `wash` is run, it starts a shell as a child process where all the action happens. It then stays running as a daemon to handle Wash-specific requests through FUSE and the socket API. However if it needs to prompt for input, it has trouble because it's TTY is not the same as the one handling input (the child shell). We previously addressed that by temporarily transferring control of input to the Wash daemon, but that had some problems - it only worked when a plugin requested input using Wash's helper - if the process that triggered the plugin activity tried to do something with stdin - like MacVim appears to on startup - it would fail Reparent the Wash daemon to have a new session under the new shell so they share the same TTY. Prompts for input now work wherever they're triggered from. MacVim still behaves a little strangely if you get a prompt when opening it, but I'm not sure what to do about it. Fixes puppetlabs-toy-chest#623. Signed-off-by: Michael Smith <[email protected]>
MikaelSmith
added a commit
to MikaelSmith/wash
that referenced
this pull request
Dec 2, 2019
When `wash` is run, it starts a shell as a child process where all the action happens. It then stays running as a daemon to handle Wash-specific requests through FUSE and the socket API. However if it needs to prompt for input, it has trouble because it's TTY is not the same as the one handling input (the child shell). We previously addressed that by temporarily transferring control of input to the Wash daemon, but that had some problems - it only worked when a plugin requested input using Wash's helper - if the process that triggered the plugin activity tried to do something with stdin - like MacVim appears to on startup - it would fail Reparent the Wash daemon to have a new session under the new shell so they share the same TTY. Prompts for input now work wherever they're triggered from. MacVim still behaves a little strangely if you get a prompt when opening it, but I'm not sure what to do about it. Fixes puppetlabs-toy-chest#623. Signed-off-by: Michael Smith <[email protected]>
MikaelSmith
added a commit
to MikaelSmith/wash
that referenced
this pull request
Dec 3, 2019
When `wash` is run, it starts a shell as a child process where all the action happens. It then stays running as a daemon to handle Wash-specific requests through FUSE and the socket API. However if it needs to prompt for input, it has trouble because it's TTY is not the same as the one handling input (the child shell). We previously addressed that by temporarily transferring control of input to the Wash daemon, but that had some problems - it only worked when a plugin requested input using Wash's helper - if the process that triggered the plugin activity tried to do something with stdin - like MacVim appears to on startup - it would fail Reparent the Wash daemon to have a new session under the new shell so they share the same TTY. Prompts for input now work wherever they're triggered from. MacVim still behaves a little strangely if you get a prompt when opening it, but I'm not sure what to do about it. Fixes puppetlabs-toy-chest#623. Signed-off-by: Michael Smith <[email protected]>
MikaelSmith
added a commit
to MikaelSmith/wash
that referenced
this pull request
Dec 3, 2019
When `wash` is run, it starts a shell as a child process where all the action happens. It then stays running as a daemon to handle Wash-specific requests through FUSE and the socket API. However if it needs to prompt for input, it has trouble because it's TTY is not the same as the one handling input (the child shell). We previously addressed that by temporarily transferring control of input to the Wash daemon, but that had some problems - it only worked when a plugin requested input using Wash's helper - if the process that triggered the plugin activity tried to do something with stdin - like MacVim appears to on startup - it would fail Reparent the Wash daemon to have a new session under the new shell so they share the same TTY. Prompts for input now work wherever they're triggered from. MacVim still behaves a little strangely if you get a prompt when opening it, but I'm not sure what to do about it. Fixes puppetlabs-toy-chest#623. Signed-off-by: Michael Smith <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sometimes a plugin needs control of the console for a prompt it doesn't make directly. Get control before calling all primitives that take a context (as a proxy for potential network activity or execs).
This isn't entirely safe. If it's backgrounded and prompts for input it may not exit cleanly. Using
plugin.Promptguards against this issue.Fixes #297.
Signed-off-by: Michael Smith [email protected]