Merge into trunk : 022-destroyer : Code : juju-core

Status:

Merged

Approved by:

Nate Finch on 2013-11-04

Approved revision:

no longer in the source branch.

Merged at revision:

2022

Proposed branch:

lp://staging/~natefinch/juju-core/022-destroyer

Merge into:

lp://staging/~go-bot/juju-core/trunk

Diff against target:

187 lines (+47/-22)

2 files modified

cmd/juju/cmd_test.go (+15/-12)
cmd/juju/destroyenvironment.go (+32/-10)

To merge this branch:

bzr merge lp://staging/~natefinch/juju-core/022-destroyer

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Juju Engineering		2013-10-16	Pending
Review via email: mp+191501@code.staging.launchpad.net

Commit message

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.

Description of the change

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/

Revision history for this message

Nate Finch (natefinch) wrote on 2013-10-16:

#

Download full text (7.0 KiB)

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...

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: nate.finch@canonical.com-20131016202642-ba284prdxs23pnp4

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:]

Revision history for this message

Roger Peppe (rogpeppe) wrote on 2013-10-17:

#

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/

Revision history for this message

Nate Finch (natefinch) wrote on 2013-10-17:

#

Please take a look.

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

Revision history for this message

Nate Finch (natefinch) wrote on 2013-10-17:

#

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/

Revision history for this message

Tim Penhey (thumper) wrote on 2013-10-31:

#

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/

Revision history for this message

Nate Finch (natefinch) wrote on 2013-11-04:

#

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/

juju-core

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

Commit message

Description of the change

Preview Diff

Subscribers