-
Notifications
You must be signed in to change notification settings - Fork 662
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
client/shell: Start instance if it's not running #631
Conversation
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.
Works ok, but I've concerns about the UI, noted inline
Also, no tests?
Regarding tests, yeah, we need to do a bunch of work in these types of places to enable testing, ie, mocking many things, etc. It's something we really, really need to do, but this is not the place to do it. |
Re: tests, ok |
b25f06d
to
f9b67f3
Compare
Also, use original message when instance is not running. Based on review feedback
f9b67f3
to
331dcd1
Compare
@gerboland, this is ready for another review pass. Thanks! |
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.
Code looks ok, and basic testing it works fine.
Only niggle is if you do "shell" for a stopped VM, then hit Ctrl+C, then try "shell" again, it just hangs... Not for this PR tho.
bors r+
631: client/shell: Start instance if it's not running r=gerboland a=townsend2010 Fixes #621 Co-authored-by: Chris Townsend <[email protected]>
Build failed |
Fixes #621