Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(615)

Issue 60630043: Fix for bug 1246938: ssh exit status lost

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 1 month ago by abel.deuring
Modified:
10 years, 1 month ago
Reviewers:
axw, natefinch, mp+205160
Visibility:
Public.

Description

Fix for bug 1246938: ssh exit status lost Bug 1246938 sugegsts to use the exit code from an ssh subprocess as the exit status of juju itself. "juju run" already has some provisions for this behavior; I added a similar treatment of the result of c.Wait() to Cmd.Run() in utils/ssh/ssh.go This function now returns an rcPassthroughError when the subprocess properly exits but returns a non-zero status code. The function SuperCommand.Run() just logged any error and replaced it with ErrSilent, thus spoiling the whole purpose of rcPassthroughError. I changed this function so rcPassthroughError is no longer replaced. This changes the behaviour of "juju run" as well: juju run --machine 1 "ls /asd" ; echo $? ls: cannot access /asd: No such file or directory ERROR rc: 2 2 juju ssh 1 ls /asd ; echo $? ls: cannot access /asd: No such file or directory Connection to 10.0.3.195 closed. ERROR rc: 2 2 https://code.launchpad.net/~adeuring/juju-core/1246938/+merge/205160 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 4

Patch Set 2 : Fix for bug 1246938: ssh exit status lost #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+27 lines, -2 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/supercommand.go View 1 chunk +3 lines, -1 line 0 comments Download
M utils/ssh/ssh.go View 1 2 chunks +12 lines, -1 line 1 comment Download
M utils/ssh/ssh_test.go View 2 chunks +10 lines, -0 lines 0 comments Download

Messages

Total messages: 6
abel.deuring
Please take a look.
10 years, 1 month ago (2014-02-06 13:47:46 UTC) #1
natefinch
Needs a couple tweaks, but the general idea looks good. https://codereview.appspot.com/60630043/diff/1/utils/ssh/ssh.go File utils/ssh/ssh.go (right): https://codereview.appspot.com/60630043/diff/1/utils/ssh/ssh.go#newcode15 ...
10 years, 1 month ago (2014-02-06 14:05:16 UTC) #2
abel.deuring
https://codereview.appspot.com/60630043/diff/1/utils/ssh/ssh.go File utils/ssh/ssh.go (right): https://codereview.appspot.com/60630043/diff/1/utils/ssh/ssh.go#newcode15 utils/ssh/ssh.go:15: "launchpad.net/juju-core/cmd" On 2014/02/06 14:05:17, nate.finch wrote: > please separate ...
10 years, 1 month ago (2014-02-07 11:08:40 UTC) #3
abel.deuring
Please take a look.
10 years, 1 month ago (2014-02-07 11:09:17 UTC) #4
natefinch
LGTM. Glad OS-specific code actually wasn't actually needed. It seems likely that the code will ...
10 years, 1 month ago (2014-02-07 13:13:39 UTC) #5
axw
10 years, 1 month ago (2014-02-10 02:25:34 UTC) #6
https://codereview.appspot.com/60630043/diff/20001/utils/ssh/ssh.go
File utils/ssh/ssh.go (right):

https://codereview.appspot.com/60630043/diff/20001/utils/ssh/ssh.go#newcode18
utils/ssh/ssh.go:18: "launchpad.net/juju-core/cmd"
It seems really wrong to me that utils/ssh is importing cmd. I would prefer if
the passthrough-error stuff were moved to utils, or if a utils/ssh-specific
error type were introduced and checked inside cmd/juju/ssh.go.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b