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

[Andrew Wilkins]

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/

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/

[Andrew Wilkins (axwalk)]

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

[William Reade (fwereade)]

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/

[Andrew Wilkins (axwalk)]

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/

[William Reade (fwereade)]

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/

[Andrew Wilkins (axwalk)]

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/

=== 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:44:23 +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
 	}
 

=== 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:44:23 +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