Code review comment for lp:~natefinch/juju-core/022-destroyer

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.DefaultContext()
@@ -217,12 +209,12 @@

   assertEnvironNotDestroyed(c, env, store)

- // Ensure confirmation is requested if "-y" is not specified.
+ // Ensure confirmation is requested
   stdin.WriteString("n")
   opc, errc := runCommand(ctx, new(DestroyEnvironmentCommand))
   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.
@@ -233,17 +225,8 @@
   c.Check(<-errc, gc.ErrorMatches, "Environment destruction aborted")
   assertEnvironNotDestroyed(c, env, store)

- // "--yes" passed: no confirmation request.
- stdin.Reset()
- stdout.Reset()
- opc, errc = runCommand(ctx, new(DestroyEnvironmentCommand), "--yes")
- c.Check(<-errc, gc.IsNil)
- c.Check((<-opc).(dummy.OpDestroy).Env, gc.Equals, "peckham")
- c.Check(stdout.String(), gc.Equals, "")
- assertEnvironDestroyed(c, env, store)
-
- // Any of casing of "y" and "yes" will confirm.
- for _, answer := range []string{"y", "Y", "yes", "YES"} {
+ // "destroy <envname>" will confirm
+ for _, answer := range []string{"destroy peckham", "DESTROY PECKHAM"} {
    // Prepare the environment so we can destroy it.
    _, err := environs.PrepareFromName("", store)
    c.Assert(err, gc.IsNil)
@@ -254,7 +237,7 @@
    opc, errc = runCommand(ctx, new(DestroyEnvironmentCommand))
    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)
   }
  }

Index: cmd/juju/destroyenvironment.go
=== modified file 'cmd/juju/destroyenvironment.go'
--- cmd/juju/destroyenvironment.go 2013-10-02 10:34:22 +0000
+++ cmd/juju/destroyenvironment.go 2013-10-16 20:26:42 +0000
@@ -4,8 +4,10 @@
  package main

  import (
+ "bufio"
   "errors"
   "fmt"
+ "io"
   "strings"

   "launchpad.net/gnuflag"
@@ -18,7 +20,6 @@
  // DestroyEnvironmentCommand destroys an environment.
  type DestroyEnvironmentCommand struct {
   cmd.EnvCommandBase
- assumeYes bool
  }

  func (c *DestroyEnvironmentCommand) Info() *cmd.Info {
@@ -30,8 +31,6 @@

  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 {
@@ -43,14 +42,19 @@
   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")
- }
+
+ fmt.Fprintf(ctx.Stdout, destroyEnvMsg, environ.Name(),
environ.Config().Type(), environ.Name())
+
+ scanner := bufio.NewScanner(ctx.Stdin)
+ scanner.Scan()
+ err = scanner.Err()
+ if err != nil && err != io.EOF {
+ return fmt.Errorf("Environment destruction aborted. Error reading
input: %s", scanner.Err())
+ }
+
+ answer := strings.ToLower(scanner.Text())
+ if answer != strings.ToLower("destroy "+environ.Name()) {
+ return errors.New("Environment destruction aborted")
   }

   // TODO(axw) 2013-08-30 bug 1218688
@@ -60,8 +64,8 @@
   return environs.Destroy(environ, store)
  }

-const destroyEnvMsg = `
-WARNING: this command will destroy the %q environment (type: %s)
+var destroyEnvMsg = `
+WARNING! This command will destroy the %q environment (type: %s)
  This includes all machines, services, data and other resources.

-Continue [y/N]? `
+Type "destroy %s" to continue: `[1:]