Merge lp:~natefinch/juju-core/022-destroyer into lp:~go-bot/juju-core/trunk

[Nate Finch]

Destroy environment now requires that you always type the name of the environment you're destroying, it will not use the default or current environment. This is to prevent people from accidentally destroying the wrong environment, since no one really reads the warning messages, and destroy environment is pretty cataclysmic.

Add safety belt to destroy-environment

Destroy-environment is dangerous and we should prevent people from shooting themselves in the foot if at all possible. This change removes the -y flag from destroy-environment and changes the "are you sure?" prompt to force the user to type "destroy <environment_name>". The flag is too easy to pass without ththinking, and the y at the prompt is similarly too easy to type in without the user actually thinking about what they're doing. Forcing them to type the environment name will also ensure they don't accidentally destroy the wrong environment by accident.

https://codereview.appspot.com/14502070/

[Nate Finch (natefinch)]

Reviewers: mp+191501_code.launchpad.net,

Message:
Please take a look.

Description:
Add safety belt to destroy-environment

Destroy-environment is dangerous and we should prevent people from
shooting themselves in the foot if at all possible. This change removes
the -y flag from destroy-environment and changes the "are you sure?"
prompt to force the user to type "destroy <environment_name>". The flag
is too easy to pass without ththinking, and the y at the prompt is
similarly too easy to type in without the user actually thinking about
what they're doing. Forcing them to type the environment name will also
ensure they don't accidentally destroy the wrong environment by
accident.

https://code.launchpad.net/~natefinch/juju-core/022-destroyer/+merge/191501

(do not edit description out of merge proposal)

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

Affected files (+33, -44 lines):
   A [revision details]
   M cmd/juju/cmd_test.go
   M cmd/juju/destroyenvironment.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-20131016154732-13eh4uj8dhsfc1mw
+New revision: <email address hidden>

Index: cmd/juju/cmd_test.go
=== modified file 'cmd/juju/cmd_test.go'
--- cmd/juju/cmd_test.go 2013-10-10 23:17:43 +0000
+++ cmd/juju/cmd_test.go 2013-10-16 20:26:42 +0000
@@ -160,6 +160,10 @@
  }

  func (*CmdSuite) TestDestroyEnvironmentCommand(c *gc.C) {
+ var stdin bytes.Buffer
+ ctx := cmd.DefaultContext()
+ ctx.Stdin = &stdin
+
   // Prepare the environment so we can destroy it.
   store, err := configstore.Default()
   c.Assert(err, gc.IsNil)
@@ -171,7 +175,8 @@
   c.Assert(err, gc.IsNil)

   // normal destroy
- opc, errc := runCommand(nullContext(),
new(DestroyEnvironmentCommand), "--yes")
+ stdin.WriteString("destroy peckham")
+ opc, errc := runCommand(ctx, new(DestroyEnvironmentCommand))
   c.Check(<-errc, gc.IsNil)
   c.Check((<-opc).(dummy.OpDestroy).Env, gc.Equals, "peckham")

@@ -183,26 +188,13 @@
   c.Assert(err, gc.IsNil)

   // destroy with broken environment
- opc, errc = runCommand(nullContext(),
new(DestroyEnvironmentCommand), "--yes", "-e", "brokenenv")
+ stdin.WriteString("destroy brokenenv")
+ opc, errc = runCommand(ctx,
new(DestroyEnvironmentCommand), "-e", "brokenenv")
   c.Check(<-opc, gc.IsNil)
   c.Check(<-errc, gc.ErrorMatches, "dummy.Destroy is broken")
   c.Check(<-opc, gc.IsNil)
  }

-func (*CmdSuite) TestDestroyEnvironmentCommandConfirmationFlag(c *gc.C) {
- com := new(DestroyEnvironmentCommand)
- c.Check(coretesting.InitCommand(com, nil), gc.IsNil)
- c.Check(com.assumeYes, gc.Equals, false)
-
- com = new(DestroyEnvironmentCommand)
- c.Check(coretesting.InitCommand(com, []string{"-y"}), gc.IsNil)
- c.Check(com.assumeYes, gc.Equals, true)
-
- com = new(DestroyEnvironmentCommand)
- c.Check(coretesting.InitCommand(com, []string{"--yes"}), gc.IsNil)
- c.Check(com.assumeYes, gc.Equals, true)
-}
-
  func (*CmdSuite) TestDestroyEnvironmentCommandConfirmation(c *gc.C) {
   var stdin, stdout bytes.Buffer
   ctx := cmd.DefaultContex...

[Roger Peppe (rogpeppe)]

On 2013/10/16 20:50:46, nate.finch wrote:
> Please take a look.

Please no!

I want to be able to use juju easily in scripts. This is
Unix and our target audience are developers and admins. Please
allow us to do dangerous things on the command line.

https://codereview.appspot.com/14502070/

[Nate Finch (natefinch)]

Please take a look.

https://codereview.appspot.com/14502070/

[Nate Finch (natefinch)]

I updated the code so that the -y flag is back, but you must always
specify the environment... it'll never take the current or default
environment.

The format of the command is now:

juju destroy-environment <envname> [-y|--yes]

Note, I chose not to use the normal -e environment flag because a flag
implies it's optional (which it's not).

https://codereview.appspot.com/14502070/

[Tim Penhey (thumper)]

LGTM

I think this is an overall improvement to the UI. Still scriptable IMO.

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

https://codereview.appspot.com/14502070/diff/5001/cmd/juju/destroyenvironment.go#newcode54
cmd/juju/destroyenvironment.go:54: scanner :=
bufio.NewScanner(ctx.Stdin)
Just curious, but what benefit do we get over using bufio.NewScanner
over fmt.Fscanln?

https://codereview.appspot.com/14502070/

[Nate Finch (natefinch)]

On 2013/10/31 22:06:09, thumper wrote:
> LGTM

> I think this is an overall improvement to the UI. Still scriptable
IMO.

https://codereview.appspot.com/14502070/diff/5001/cmd/juju/destroyenvironment.go
> File cmd/juju/destroyenvironment.go (right):

https://codereview.appspot.com/14502070/diff/5001/cmd/juju/destroyenvironment.go#newcode54
> cmd/juju/destroyenvironment.go:54: scanner :=
bufio.NewScanner(ctx.Stdin)
> Just curious, but what benefit do we get over using bufio.NewScanner
over
> fmt.Fscanln?

  fmt.Fscanln actually fails to return the entire string, and returns an
error about no EOL. We were ignoring it, but it bit me when I was
twiddling with the code and tried to get it to read the full line... so
I figured it was better to make the code a good example rather than a
bad example, even if functionally it behaved the same in this particular
case.

https://codereview.appspot.com/14502070/

=== modified file 'cmd/juju/cmd_test.go'
--- cmd/juju/cmd_test.go	2013-10-10 23:17:43 +0000
+++ cmd/juju/cmd_test.go	2013-10-17 17:43:53 +0000
@@ -88,7 +88,6 @@
 // flags, and that extra arguments will cause parsing to fail.
 var EnvironmentInitTests = []func() (cmd.Command, []string){
 	func() (cmd.Command, []string) { return new(BootstrapCommand), nil },
-	func() (cmd.Command, []string) { return new(DestroyEnvironmentCommand), nil },
 	func() (cmd.Command, []string) {
 		return new(DeployCommand), []string{"charm-name", "service-name"}
 	},
@@ -170,8 +169,12 @@
 	_, err = store.ReadInfo("peckham")
 	c.Assert(err, gc.IsNil)
 
+	// check environment is mandatory
+	opc, errc := runCommand(nullContext(), new(DestroyEnvironmentCommand))
+	c.Check(<-errc, gc.Equals, NoEnvironmentError)
+
 	// normal destroy
-	opc, errc := runCommand(nullContext(), new(DestroyEnvironmentCommand), "--yes")
+	opc, errc = runCommand(nullContext(), new(DestroyEnvironmentCommand), "peckham", "--yes")
 	c.Check(<-errc, gc.IsNil)
 	c.Check((<-opc).(dummy.OpDestroy).Env, gc.Equals, "peckham")
 
@@ -183,7 +186,7 @@
 	c.Assert(err, gc.IsNil)
 
 	// destroy with broken environment
-	opc, errc = runCommand(nullContext(), new(DestroyEnvironmentCommand), "--yes", "-e", "brokenenv")
+	opc, errc = runCommand(nullContext(), new(DestroyEnvironmentCommand), "brokenenv", "--yes")
 	c.Check(<-opc, gc.IsNil)
 	c.Check(<-errc, gc.ErrorMatches, "dummy.Destroy is broken")
 	c.Check(<-opc, gc.IsNil)
@@ -191,15 +194,15 @@
 
 func (*CmdSuite) TestDestroyEnvironmentCommandConfirmationFlag(c *gc.C) {
 	com := new(DestroyEnvironmentCommand)
-	c.Check(coretesting.InitCommand(com, nil), gc.IsNil)
+	c.Check(coretesting.InitCommand(com, []string{"peckham"}), gc.IsNil)
 	c.Check(com.assumeYes, gc.Equals, false)
 
 	com = new(DestroyEnvironmentCommand)
-	c.Check(coretesting.InitCommand(com, []string{"-y"}), gc.IsNil)
+	c.Check(coretesting.InitCommand(com, []string{"peckham", "-y"}), gc.IsNil)
 	c.Check(com.assumeYes, gc.Equals, true)
 
 	com = new(DestroyEnvironmentCommand)
-	c.Check(coretesting.InitCommand(com, []string{"--yes"}), gc.IsNil)
+	c.Check(coretesting.InitCommand(com, []string{"peckham", "--yes"}), gc.IsNil)
 	c.Check(com.assumeYes, gc.Equals, true)
 }
 
@@ -219,16 +222,16 @@
 
 	// Ensure confirmation is requested if "-y" is not specified.
 	stdin.WriteString("n")
-	opc, errc := runCommand(ctx, new(DestroyEnvironmentCommand))
+	opc, errc := runCommand(ctx, new(DestroyEnvironmentCommand), "peckham")
 	c.Check(<-errc, gc.ErrorMatches, "Environment destruction aborted")
 	c.Check(<-opc, gc.IsNil)
-	c.Check(stdout.String(), gc.Matches, "WARNING:.*peckham.*\\(type: dummy\\)(.|\n)*")
+	c.Check(stdout.String(), gc.Matches, "WARNING!.*peckham.*\\(type: dummy\\)(.|\n)*")
 	assertEnvironNotDestroyed(c, env, store)
 
 	// EOF on stdin: equivalent to answering no.
 	stdin.Reset()
 	stdout.Reset()
-	opc, errc = runCommand(ctx, new(DestroyEnvironmentCommand))
+	opc, errc = runCommand(ctx, new(DestroyEnvironmentCommand), "peckham")
 	c.Check(<-opc, gc.IsNil)
 	c.Check(<-errc, gc.ErrorMatches, "Environment destruction aborted")
 	assertEnvironNotDestroyed(c, env, store)
@@ -236,7 +239,7 @@
 	// "--yes" passed: no confirmation request.
 	stdin.Reset()
 	stdout.Reset()
-	opc, errc = runCommand(ctx, new(DestroyEnvironmentCommand), "--yes")
+	opc, errc = runCommand(ctx, new(DestroyEnvironmentCommand), "peckham", "--yes")
 	c.Check(<-errc, gc.IsNil)
 	c.Check((<-opc).(dummy.OpDestroy).Env, gc.Equals, "peckham")
 	c.Check(stdout.String(), gc.Equals, "")
@@ -251,10 +254,10 @@
 		stdin.Reset()
 		stdout.Reset()
 		stdin.WriteString(answer)
-		opc, errc = runCommand(ctx, new(DestroyEnvironmentCommand))
+		opc, errc = runCommand(ctx, new(DestroyEnvironmentCommand), "peckham")
 		c.Check(<-errc, gc.IsNil)
 		c.Check((<-opc).(dummy.OpDestroy).Env, gc.Equals, "peckham")
-		c.Check(stdout.String(), gc.Matches, "WARNING:.*peckham.*\\(type: dummy\\)(.|\n)*")
+		c.Check(stdout.String(), gc.Matches, "WARNING!.*peckham.*\\(type: dummy\\)(.|\n)*")
 		assertEnvironDestroyed(c, env, store)
 	}
 }

=== modified file 'cmd/juju/destroyenvironment.go'
--- cmd/juju/destroyenvironment.go	2013-10-02 10:34:22 +0000
+++ cmd/juju/destroyenvironment.go	2013-10-17 17:43:53 +0000
@@ -4,8 +4,10 @@
 package main
 
 import (
+	"bufio"
 	"errors"
 	"fmt"
+	"io"
 	"strings"
 
 	"launchpad.net/gnuflag"
@@ -15,21 +17,24 @@
 	"launchpad.net/juju-core/environs/configstore"
 )
 
+var NoEnvironmentError = errors.New("no environment specified")
+
 // DestroyEnvironmentCommand destroys an environment.
 type DestroyEnvironmentCommand struct {
-	cmd.EnvCommandBase
+	cmd.CommandBase
+	envName   string
 	assumeYes bool
 }
 
 func (c *DestroyEnvironmentCommand) Info() *cmd.Info {
 	return &cmd.Info{
 		Name:    "destroy-environment",
+		Args:    "<environment name>",
 		Purpose: "terminate all machines and other associated resources for an environment",
 	}
 }
 
 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, "")
 }
@@ -39,15 +44,20 @@
 	if err != nil {
 		return fmt.Errorf("cannot open environment info storage: %v", err)
 	}
-	environ, err := environs.NewFromName(c.EnvName, store)
+	environ, err := environs.NewFromName(c.envName, store)
 	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)
+		fmt.Fprintf(ctx.Stdout, destroyEnvMsg, environ.Name(), environ.Config().Type())
+
+		scanner := bufio.NewScanner(ctx.Stdin)
+		scanner.Scan()
+		err := scanner.Err()
+		if err != nil && err != io.EOF {
+			return fmt.Errorf("Environment destruction aborted: %s", err)
+		}
+		answer := strings.ToLower(scanner.Text())
 		if answer != "y" && answer != "yes" {
 			return errors.New("Environment destruction aborted")
 		}
@@ -60,8 +70,20 @@
 	return environs.Destroy(environ, store)
 }
 
-const destroyEnvMsg = `
-WARNING: this command will destroy the %q environment (type: %s)
+func (c *DestroyEnvironmentCommand) Init(args []string) error {
+	switch len(args) {
+	case 0:
+		return NoEnvironmentError
+	case 1:
+		c.envName = args[0]
+		return nil
+	default:
+		return cmd.CheckEmpty(args[1:])
+	}
+}
+
+var destroyEnvMsg = `
+WARNING! this command will destroy the %q environment (type: %s)
 This includes all machines, services, data and other resources.
 
-Continue [y/N]? `
+Continue [y/N]? `[1:]