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.

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

Exec Sh Feature #151

Merged
merged 7 commits into from
May 12, 2019
Merged

Exec Sh Feature #151

merged 7 commits into from
May 12, 2019

Conversation

fr05t1k
Copy link
Contributor

@fr05t1k fr05t1k commented Oct 10, 2018

@fr05t1k fr05t1k mentioned this pull request Oct 10, 2018
@bcicen
Copy link
Owner

bcicen commented Oct 10, 2018

This PR only works if the user has Docker on their local machine + the target container contains bash.

A much more acceptable approach would be implementing an arbitrary container exec feature in the connectors subpackage using the go-dockerclient library.

@fr05t1k
Copy link
Contributor Author

fr05t1k commented Oct 12, 2018

@bcicen ok, I'll try a bit later. Anyway it works for my case :)

@pjvds
Copy link

pjvds commented Oct 19, 2018

executing the default shell is already a great step forward! 👍

@fr05t1k
Copy link
Contributor Author

fr05t1k commented Oct 20, 2018

@bcicen I used API for executing but left sh command. I can add a check for global env variable and local env variable for overriding default shell. Like in Kitematic:
https://github.com/docker/kitematic/blob/master/src/components/ContainerDetailsSubheader.react.js#L100

menus.go Outdated

ui.DefaultEvtStream.ResetHandlers()
defer ui.DefaultEvtStream.ResetHandlers()
ui.StopLoop()
Copy link
Owner

Choose a reason for hiding this comment

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

StopLoop() and Loop() are already handled when a menu is selected from Display(), so not necessary here

@bcicen
Copy link
Owner

bcicen commented Oct 27, 2018

Looking much better!

Rather than forcing a default shell globally for all containers, it might make more sense to make this a generic exec menu option/method, with a second input menu presented, pre-filled with the default shell, but allowing the user to modify/input a custom command as well.

@fr05t1k
Copy link
Contributor Author

fr05t1k commented Oct 27, 2018

Looking much better!

Rather than forcing a default shell globally for all containers, it might make more sense to make this a generic exec menu option/method, with a second input menu presented, pre-filled with the default shell, but allowing the user to modify/input a custom command as well.

Good idea. I'll do it.

BTW there are still some artifacts with terminal rendering through direct stdout. I'm trying to solve it.

@bcicen
Copy link
Owner

bcicen commented May 12, 2019

FYI, after making some updates to handle potentially empty frames when first attached, I've merged your branch here. Thanks again!

@fr05t1k
Copy link
Contributor Author

fr05t1k commented May 13, 2019

It still has artifacts. I don't think that this feature is ready for production.

Artifacts which I found:

  • terminal cursor is not blinking
  • screen size isn't correct
  • some events propagate to ctop itself

@fr05t1k fr05t1k mentioned this pull request May 13, 2019
@bcicen
Copy link
Owner

bcicen commented May 13, 2019

screen resizing might be difficult without stopping termui entirely -- if possible, it might be ideal to close and reopen std{out,in} descriptors when entering and leaving each exec session, re-initializing termui entirely

@bcicen
Copy link
Owner

bcicen commented May 14, 2019

in addition to artifacts, most combination key presses are being filtered from the stdin stream as well. The above mentioned solution of handing full stream control over to the exec thread is likely the only option that will handle all the edge cases that come with subprocess shell usage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add exec
3 participants