Merge lp:~axwalk/juju-core/lp1121914-confirm-destroy-environment into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

cmd/juju: confirm destroy-environment

Fixes bug 1121914

https://codereview.appspot.com/12158043/

cmd/juju: confirm destroy-environment

Fixes bug 1121914

https://codereview.appspot.com/12158043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+177820_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/juju: confirm destroy-environment

https://code.launchpad.net/~axwalk/juju-core/lp1121914-confirm-destroy-environment/+merge/177820

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M cmd/juju/bootstrap_test.go
   M cmd/juju/cmd_test.go
   M cmd/juju/destroyenvironment.go

[William Reade (fwereade)]

LGTM, thank you.

https://codereview.appspot.com/12158043/

[Roger Peppe (rogpeppe)]

LGTM for backwards compatibility, despite
my meh about the functionality itself.

Please reference bug 1121914 in the description
and attach it to the merge proposal.

https://codereview.appspot.com/12158043/diff/1/cmd/juju/destroyenvironment.go
File cmd/juju/destroyenvironment.go (right):

https://codereview.appspot.com/12158043/diff/1/cmd/juju/destroyenvironment.go#newcode32
cmd/juju/destroyenvironment.go:32: f.BoolVar(&c.assumeYes, "yes", false,
"Do not ask for confirmation")
You can delete the flag description from this line - it's taken from
the shortest alias.

https://codereview.appspot.com/12158043/diff/1/cmd/juju/destroyenvironment.go#newcode51
cmd/juju/destroyenvironment.go:51: return nil
I think this should probably return an error here.
The destroy-environment command has not succeeded.

https://codereview.appspot.com/12158043/diff/1/cmd/juju/destroyenvironment.go#newcode62
cmd/juju/destroyenvironment.go:62: Continue [y/N]? `
s/N/n/ ?

https://codereview.appspot.com/12158043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/12158043/

[Go Bot (go-bot)]

Attempt to merge into lp:juju-core failed due to conflicts:

text conflict in cmd/juju/destroyenvironment.go

[John A Meinel (jameinel)]

https://codereview.appspot.com/12158043/diff/1/cmd/juju/destroyenvironment.go
File cmd/juju/destroyenvironment.go (right):

https://codereview.appspot.com/12158043/diff/1/cmd/juju/destroyenvironment.go#newcode62
cmd/juju/destroyenvironment.go:62: Continue [y/N]? `
On 2013/08/01 01:34:32, axw1 wrote:
> On 2013/07/31 13:41:26, rog wrote:
> > s/N/n/ ?

> I don't think so. It's taken straight from pyjuju, and capitalising
the default
> option is fairly standard IMHO. If there's a preferred way of
conveying the
> default (e.g. "[y/n] (default: n)?") then I'm happy to do a follow-up.

I agree. It is standard practice to capitalize the default option.

https://codereview.appspot.com/12158043/

=== modified file 'cmd/juju/bootstrap_test.go'
--- cmd/juju/bootstrap_test.go	2013-07-30 15:24:12 +0000
+++ cmd/juju/bootstrap_test.go	2013-08-06 05:26:58 +0000
@@ -108,7 +108,7 @@
 	}
 
 	// Run command and check for uploads.
-	opc, errc := runCommand(new(BootstrapCommand), test.args...)
+	opc, errc := runCommand(nil, new(BootstrapCommand), test.args...)
 	if uploadCount > 0 {
 		for i := 0; i < uploadCount; i++ {
 			c.Check((<-opc).(dummy.OpPutFile).Env, gc.Equals, "peckham")

=== modified file 'cmd/juju/cmd_test.go'
--- cmd/juju/cmd_test.go	2013-07-31 04:52:39 +0000
+++ cmd/juju/cmd_test.go	2013-08-06 05:26:58 +0000
@@ -4,6 +4,7 @@
 package main
 
 import (
+	"bytes"
 	"os"
 	"reflect"
 
@@ -118,7 +119,7 @@
 	}
 }
 
-func runCommand(com cmd.Command, args ...string) (opc chan dummy.Operation, errc chan error) {
+func runCommand(ctx *cmd.Context, com cmd.Command, args ...string) (opc chan dummy.Operation, errc chan error) {
 	errc = make(chan error, 1)
 	opc = make(chan dummy.Operation, 200)
 	dummy.Listen(opc)
@@ -132,7 +133,10 @@
 			return
 		}
 
-		err = com.Run(cmd.DefaultContext())
+		if ctx == nil {
+			ctx = cmd.DefaultContext()
+		}
+		err = com.Run(ctx)
 		errc <- err
 	}()
 	return
@@ -140,17 +144,69 @@
 
 func (*CmdSuite) TestDestroyEnvironmentCommand(c *C) {
 	// normal destroy
-	opc, errc := runCommand(new(DestroyEnvironmentCommand))
+	opc, errc := runCommand(nil, new(DestroyEnvironmentCommand), "--yes")
 	c.Check(<-errc, IsNil)
 	c.Check((<-opc).(dummy.OpDestroy).Env, Equals, "peckham")
 
 	// destroy with broken environment
-	opc, errc = runCommand(new(DestroyEnvironmentCommand), "-e", "brokenenv")
+	opc, errc = runCommand(nil, new(DestroyEnvironmentCommand), "--yes", "-e", "brokenenv")
 	c.Check(<-opc, IsNil)
 	c.Check(<-errc, ErrorMatches, "dummy.Destroy is broken")
 	c.Check(<-opc, IsNil)
 }
 
+func (*CmdSuite) TestDestroyEnvironmentCommandConfirmation(c *C) {
+	com := new(DestroyEnvironmentCommand)
+	c.Check(coretesting.InitCommand(com, nil), IsNil)
+	c.Check(com.assumeYes, Equals, false)
+
+	com = new(DestroyEnvironmentCommand)
+	c.Check(coretesting.InitCommand(com, []string{"-y"}), IsNil)
+	c.Check(com.assumeYes, Equals, true)
+
+	com = new(DestroyEnvironmentCommand)
+	c.Check(coretesting.InitCommand(com, []string{"--yes"}), IsNil)
+	c.Check(com.assumeYes, Equals, true)
+
+	var stdin, stdout bytes.Buffer
+	ctx := cmd.DefaultContext()
+	ctx.Stdout = &stdout
+	ctx.Stdin = &stdin
+
+	// Ensure confirmation is requested if "-y" is not specified.
+	stdin.WriteString("n")
+	opc, errc := runCommand(ctx, new(DestroyEnvironmentCommand))
+	c.Check(<-errc, ErrorMatches, "Environment destruction aborted")
+	c.Check(<-opc, IsNil)
+	c.Check(stdout.String(), Matches, "WARNING:.*peckham.*\\(type: dummy\\)(.|\n)*")
+
+	// EOF on stdin: equivalent to answering no.
+	stdin.Reset()
+	stdout.Reset()
+	opc, errc = runCommand(ctx, new(DestroyEnvironmentCommand))
+	c.Check(<-opc, IsNil)
+	c.Check(<-errc, ErrorMatches, "Environment destruction aborted")
+
+	// "--yes" passed: no confirmation request.
+	stdin.Reset()
+	stdout.Reset()
+	opc, errc = runCommand(ctx, new(DestroyEnvironmentCommand), "--yes")
+	c.Check(<-errc, IsNil)
+	c.Check((<-opc).(dummy.OpDestroy).Env, Equals, "peckham")
+	c.Check(stdout.String(), Equals, "")
+
+	// Any of casing of "y" and "yes" will confirm.
+	for _, answer := range []string{"y", "Y", "yes", "YES"} {
+		stdin.Reset()
+		stdout.Reset()
+		stdin.WriteString(answer)
+		opc, errc = runCommand(ctx, new(DestroyEnvironmentCommand))
+		c.Check(<-errc, IsNil)
+		c.Check((<-opc).(dummy.OpDestroy).Env, Equals, "peckham")
+		c.Check(stdout.String(), Matches, "WARNING:.*peckham.*\\(type: dummy\\)(.|\n)*")
+	}
+}
+
 var deployTests = []struct {
 	args []string
 	com  *DeployCommand

=== modified file 'cmd/juju/destroyenvironment.go'
--- cmd/juju/destroyenvironment.go	2013-07-30 01:31:52 +0000
+++ cmd/juju/destroyenvironment.go	2013-08-06 05:26:58 +0000
@@ -4,6 +4,12 @@
 package main
 
 import (
+	"errors"
+	"fmt"
+	"strings"
+
+	"launchpad.net/gnuflag"
+
 	"launchpad.net/juju-core/cmd"
 	"launchpad.net/juju-core/environs"
 )
@@ -11,6 +17,7 @@
 // DestroyEnvironmentCommand destroys an environment.
 type DestroyEnvironmentCommand struct {
 	cmd.EnvCommandBase
+	assumeYes bool
 }
 
 func (c *DestroyEnvironmentCommand) Info() *cmd.Info {
@@ -20,10 +27,33 @@
 	}
 }
 
-func (c *DestroyEnvironmentCommand) Run(_ *cmd.Context) error {
+func (c *DestroyEnvironmentCommand) SetFlags(f *gnuflag.FlagSet) {
+	c.EnvCommandBase.SetFlags(f)
+	f.BoolVar(&c.assumeYes, "y", false, "Do not ask for confirmation")
+	f.BoolVar(&c.assumeYes, "yes", false, "")
+}
+
+func (c *DestroyEnvironmentCommand) Run(ctx *cmd.Context) error {
 	environ, err := environs.NewFromName(c.EnvName)
 	if err != nil {
 		return err
 	}
+
+	if !c.assumeYes {
+		var answer string
+		fmt.Fprintf(ctx.Stdout, destroyEnvMsg[1:], environ.Name(), environ.Config().Type())
+		fmt.Fscanln(ctx.Stdin, &answer) // ignore error, treat as "n"
+		answer = strings.ToLower(answer)
+		if answer != "y" && answer != "yes" {
+			return errors.New("Environment destruction aborted")
+		}
+	}
+
 	return environ.Destroy(nil)
 }
+
+const destroyEnvMsg = `
+WARNING: this command will destroy the %q environment (type: %s)
+This includes all machines, services, data and other resources.
+
+Continue [y/N]? `