Skip to content
This repository has been archived by the owner on Dec 7, 2023. It is now read-only.

Unify ssh and exec commands #580

Merged
merged 6 commits into from
Apr 20, 2020
Merged

Conversation

darkowlzz
Copy link
Contributor

This change combines ssh and exec commands to use the same code. ssh no longer
uses ssh binary on the host but uses go's crypto/ssh package to create a
terminal session.

Moves newSignerForKey, newSSHConfig and joinShellCommand functions from
exec.go to ssh.go, keeping all ssh related functions in the same file.

@darkowlzz darkowlzz requested a review from twelho as a code owner April 11, 2020 19:51
@darkowlzz
Copy link
Contributor Author

darkowlzz commented Apr 11, 2020

e2e test seems to be flaky. It ran fine on my fork https://travis-ci.com/github/darkowlzz/ignite/builds/159927085

if err := session.Wait(); err != nil {
if e, ok := err.(*ssh.ExitError); ok {
switch e.ExitStatus() {
case 130:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should simply set the return code for the deferred os.Exit

}
defer session.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

Before we defer the session.Close, we can defer an os.Exit (then they will run in reverse order)

The Exit should take a code variable:
https://stackoverflow.com/a/24601700

cmd/ignite/run/ssh.go Outdated Show resolved Hide resolved
}
} else {
if err := session.Run(joinShellCommand(command)); err != nil {
return fmt.Errorf("failed to run shell command: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please copy error checking and exit code logic for previous block here so behavior is consistent.

@stealthybox
Copy link
Contributor

Please rebase on master which now includes #581 :)

@stealthybox stealthybox self-requested a review April 13, 2020 21:55
@darkowlzz darkowlzz force-pushed the unify-ssh-exec branch 2 times, most recently from 3e1d8fb to b969e99 Compare April 18, 2020 18:58
@darkowlzz
Copy link
Contributor Author

Added a defer with os.Exit after a ssh connection is establish. Since the function will no longer be able to return any error, any error that's found after this are printed so that it's visible to the user. The exit code is set based on the exit code from the ssh session.

@darkowlzz darkowlzz force-pushed the unify-ssh-exec branch 2 times, most recently from 838f01f to 8e4053b Compare April 18, 2020 19:14
This change combines ssh and exec to use the same code. ssh no longer
uses ssh binary on the host but uses go's crypto/ssh package to create a
terminal session.

Moves newSignerForKey, newSSHConfig and joinShellCommand functions from
exec.go to ssh.go, keeping all ssh related functions in the same file.
Breaking change. Short flag -t changed from timeout to tty, similar to
the exec command.
When the ssh session ends, the actual ssh exit code should be returned.
This change adds a deferred os.Exit and sets any ssh error code received
as the exit code.
@stealthybox stealthybox merged commit 657da68 into weaveworks:master Apr 20, 2020
@luxas luxas added this to the v0.7.0 milestone Jun 2, 2020
@darkowlzz darkowlzz deleted the unify-ssh-exec branch June 8, 2020 18:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants