Merge lp:~axwalk/juju-core/lp1235716-sshstorage-sudo-prompt into lp:~go-bot/juju-core/trunk

Proposed by Andrew Wilkins
Status: Merged
Approved by: Andrew Wilkins
Approved revision: no longer in the source branch.
Merged at revision: 1973
Proposed branch: lp:~axwalk/juju-core/lp1235716-sshstorage-sudo-prompt
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 30 lines (+6/-3)
2 files modified
environs/sshstorage/storage.go (+5/-2)
environs/sshstorage/storage_test.go (+1/-1)
To merge this branch: bzr merge lp:~axwalk/juju-core/lp1235716-sshstorage-sudo-prompt
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+189526@code.launchpad.net

Commit message

environs/sshstorage: don't hide sudo prompt

Connect sudo command's stdout to os.Stdout,
and capture stderr in a bytes.Buffer. This
allows sudo prompts to be presented, while
still capturing "install" errors.

Fixes #1235716

https://codereview.appspot.com/14461044/

Description of the change

environs/sshstorage: don't hide sudo prompt

Connect sudo command's stdout to os.Stdout,
and capture stderr in a bytes.Buffer. This
allows sudo prompts to be presented, while
still capturing "install" errors.

Fixes #1235716

https://codereview.appspot.com/14461044/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :

Reviewers: mp+189526_code.launchpad.net,

Message:
Please take a look.

Description:
environs/sshstorage: don't hide sudo prompt

Connect sudo command's stdout to os.Stdout,
and capture stderr in a bytes.Buffer. This
allows sudo prompts to be presented, while
still capturing "install" errors.

Fixes #1235716

https://code.launchpad.net/~axwalk/juju-core/lp1235716-sshstorage-sudo-prompt/+merge/189526

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/14461044/

Affected files (+8, -3 lines):
   A [revision details]
   M environs/sshstorage/storage.go
   M environs/sshstorage/storage_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20131004160159-i9eoormyw24etfr4
+New revision: <email address hidden>

Index: environs/sshstorage/storage.go
=== modified file 'environs/sshstorage/storage.go'
--- environs/sshstorage/storage.go 2013-10-03 08:34:41 +0000
+++ environs/sshstorage/storage.go 2013-10-07 03:40:52 +0000
@@ -84,8 +84,11 @@

   cmd := sshCommand(host, true, fmt.Sprintf("sudo bash -c %s",
utils.ShQuote(script)))
   cmd.Stdin = os.Stdin
- if out, err := cmd.CombinedOutput(); err != nil {
- err = fmt.Errorf("failed to create storage dir: %v (%v)", err,
strings.TrimSpace(string(out)))
+ cmd.Stdout = os.Stdout // for sudo prompts/output
+ var stderr bytes.Buffer
+ cmd.Stderr = &stderr
+ if err := cmd.Run(); err != nil {
+ err = fmt.Errorf("failed to create storage dir: %v (%v)", err,
strings.TrimSpace(stderr.String()))
    return nil, err
   }

Index: environs/sshstorage/storage_test.go
=== modified file 'environs/sshstorage/storage_test.go'
--- environs/sshstorage/storage_test.go 2013-10-03 08:44:30 +0000
+++ environs/sshstorage/storage_test.go 2013-10-07 03:40:52 +0000
@@ -162,7 +162,7 @@
    invocations++
    switch invocations {
    case 1, 3:
- return exec.Command("bash", "-c", "echo alles gut")
+ return exec.Command("true")
    case 2:
     // Note: must close stdin before responding the first time, or
     // the second command will race with closing stdin, and may

Revision history for this message
William Reade (fwereade) wrote :

https://codereview.appspot.com/14461044/diff/1/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/14461044/diff/1/environs/sshstorage/storage.go#newcode87
environs/sshstorage/storage.go:87: cmd.Stdout = os.Stdout // for sudo
prompts/output
We should not be using these. Sorry, I must have missed it before; but
if you're running in a command you should have a cmd.Context available
and should be using in/out/err from that.

This should in general make it an awful lot easier to test, too.

https://codereview.appspot.com/14461044/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

https://codereview.appspot.com/14461044/diff/1/environs/sshstorage/storage.go
File environs/sshstorage/storage.go (right):

https://codereview.appspot.com/14461044/diff/1/environs/sshstorage/storage.go#newcode87
environs/sshstorage/storage.go:87: cmd.Stdout = os.Stdout // for sudo
prompts/output
On 2013/10/08 07:48:25, fwereade wrote:
> We should not be using these. Sorry, I must have missed it before; but
if you're
> running in a command you should have a cmd.Context available and
should be using
> in/out/err from that.

How do you get that into an Environ? Because that's where NewSSHStorage
is called.

> This should in general make it an awful lot easier to test, too.

https://codereview.appspot.com/14461044/

Revision history for this message
William Reade (fwereade) wrote :

On 2013/10/08 10:14:00, axw wrote:
> On 2013/10/08 07:48:25, fwereade wrote:
> > We should not be using these. Sorry, I must have missed it before;
but if
> you're
> > running in a command you should have a cmd.Context available and
should be
> using
> > in/out/err from that.

> How do you get that into an Environ? Because that's where
NewSSHStorage is
> called.

Ha. Sorry, I did not think that through. OK, consider this LGTM with a
tech-debt bug -- it's not worth blocking something so user-helpful over
architectural quibbling. Sinzui said he wouldn't be cutting a release
until tomorrow, so you have my blessing to squeeze this into 1.16 if you
can.

If you have any bright ideas that might help answer your question,
though, I'm all ears.

https://codereview.appspot.com/14461044/

Revision history for this message
Andrew Wilkins (axwalk) wrote :

On 2013/10/09 17:55:40, fwereade wrote:
> On 2013/10/08 10:14:00, axw wrote:
> > On 2013/10/08 07:48:25, fwereade wrote:
> > > We should not be using these. Sorry, I must have missed it before;
but if
> > you're
> > > running in a command you should have a cmd.Context available and
should be
> > using
> > > in/out/err from that.
> >
> > How do you get that into an Environ? Because that's where
NewSSHStorage is
> > called.

> Ha. Sorry, I did not think that through. OK, consider this LGTM with a
tech-debt
> bug -- it's not worth blocking something so user-helpful over
architectural
> quibbling. Sinzui said he wouldn't be cutting a release until
tomorrow, so you
> have my blessing to squeeze this into 1.16 if you can.

> If you have any bright ideas that might help answer your question,
though, I'm
> all ears.

Thanks, I'll log a bug for now, and have a think about it.

https://codereview.appspot.com/14461044/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'environs/sshstorage/storage.go'
2--- environs/sshstorage/storage.go 2013-10-03 08:34:41 +0000
3+++ environs/sshstorage/storage.go 2013-10-07 03:44:23 +0000
4@@ -84,8 +84,11 @@
5
6 cmd := sshCommand(host, true, fmt.Sprintf("sudo bash -c %s", utils.ShQuote(script)))
7 cmd.Stdin = os.Stdin
8- if out, err := cmd.CombinedOutput(); err != nil {
9- err = fmt.Errorf("failed to create storage dir: %v (%v)", err, strings.TrimSpace(string(out)))
10+ cmd.Stdout = os.Stdout // for sudo prompts/output
11+ var stderr bytes.Buffer
12+ cmd.Stderr = &stderr
13+ if err := cmd.Run(); err != nil {
14+ err = fmt.Errorf("failed to create storage dir: %v (%v)", err, strings.TrimSpace(stderr.String()))
15 return nil, err
16 }
17
18
19=== modified file 'environs/sshstorage/storage_test.go'
20--- environs/sshstorage/storage_test.go 2013-10-03 08:44:30 +0000
21+++ environs/sshstorage/storage_test.go 2013-10-07 03:44:23 +0000
22@@ -162,7 +162,7 @@
23 invocations++
24 switch invocations {
25 case 1, 3:
26- return exec.Command("bash", "-c", "echo alles gut")
27+ return exec.Command("true")
28 case 2:
29 // Note: must close stdin before responding the first time, or
30 // the second command will race with closing stdin, and may

Subscribers

People subscribed via source and target branches

to status/vote changes: