Fixes docker-archive-public#2349 - rm get user confirmation before proceeding further

askb · askb · commit 77fba72d13ed · 2015-12-02T09:17:33.000+05:30
* adds new flag `-y` prompting for user confirmation before removal * Modified existing integration tests to work with the fix docker-archive-public#2349. * Added tests for checking user confirmation, updated the test cases use sub-shell with `|` * Updated the reference docs for rm sub-command incorporated changes by @dgageot, @jeanlaurent and @nathanleclaire Signed-off-by: Anil Belur <askb23@gmail.com>
diff --git a/commands/commands.go b/commands/commands.go
@@ -239,6 +239,10 @@ var Commands = []cli.Command{
 				Name:  "force, f",
 				Usage: "Remove local configuration even if machine cannot be removed",
 			},
+			cli.BoolFlag{
+				Name:  "y",
+				Usage: "Assumes automatic yes to proceed with remove, without prompting further user confirmation",
+			},
 		},
 		Name:        "rm",
 		Usage:       "Remove a machine",
diff --git a/commands/rm.go b/commands/rm.go
@@ -14,13 +14,21 @@ func cmdRm(c CommandLine, api libmachine.API) error {
 	}
 
 	force := c.Bool("force")
+	confirm := c.Bool("y")
 
 	for _, hostName := range c.Args() {
 		h, err := api.Load(hostName)
 		if err != nil {
 			return fmt.Errorf("Error removing host %q: %s", hostName, err)
 		}
 
+		if !confirm && !force {
+			userinput, err := confirmInput(fmt.Sprintf("Do you really want to remove %q?", hostName))
+			if !userinput {
+				return err
+			}
+		}
+
 		if err := h.Driver.Remove(); err != nil {
 			if !force {
 				log.Errorf("Provider error removing machine %q: %s", hostName, err)
diff --git a/commands/rm_test.go b/commands/rm_test.go
@@ -23,6 +23,11 @@ func TestCmdRmMissingMachineName(t *testing.T) {
 func TestCmdRm(t *testing.T) {
 	commandLine := &commandstest.FakeCommandLine{
 		CliArgs: []string{"machineToRemove1", "machineToRemove2"},
+		LocalFlags: &commandstest.FakeFlagger{
+			Data: map[string]interface{}{
+				"y": true,
+			},
+		},
 	}
 	api := &libmachinetest.FakeAPI{
 		Hosts: []*host.Host{
diff --git a/docs/get-started-cloud.md b/docs/get-started-cloud.md
@@ -82,6 +82,11 @@ the last section. If we look at `docker-machine ls`, we'll see it is now the
 To remove a host and all of its containers and images, use `docker-machine rm`:
 
     $ docker-machine rm dev staging
+    Do you really want to remove "dev"? (y/n): y
+    Successfully removed dev
+    Do you really want to remove "staging"? (y/n): y
+    Successfully removed staging
+
     $ docker-machine ls
     NAME      ACTIVE   DRIVER       STATE     URL
 
diff --git a/docs/reference/rm.md b/docs/reference/rm.md
@@ -18,7 +18,11 @@ on the cloud provider or virtualization management platform.
     NAME   ACTIVE   DRIVER       STATE     URL
     foo0   -        virtualbox   Running   tcp://192.168.99.105:2376
     foo1   -        virtualbox   Running   tcp://192.168.99.106:2376
+
     $ docker-machine rm foo1
+    Do you really want to remove "foo1"? (y/n): y
+    Successfully removed foo1
+
     $ docker-machine ls
     NAME   ACTIVE   DRIVER       STATE     URL
     foo0   -        virtualbox   Running   tcp://192.168.99.105:2376
diff --git a/test/integration/cli/create-rm.bats b/test/integration/cli/create-rm.bats
@@ -64,31 +64,53 @@ load ${BASE_TEST_DIR}/helpers.bash
 }
 
 @test "none: rm with no name fails 'machine rm'" {
-  run machine rm
+  run machine rm -y
   last=$(expr ${#lines[@]} - 1)
   [ "$status" -eq 1 ]
   [[ ${lines[$last]} == "Error: Expected to get one or more machine names as arguments" ]]
 }
 
 @test "none: rm non existent machine fails 'machine rm ∞'" {
-  run machine rm ∞
+  run machine rm ∞ -y
   [ "$status" -eq 1 ]
   [[ ${lines[0]} == "Error removing host \"∞\": Host does not exist: \"∞\"" ]]
 }
 
 @test "none: rm is successful 'machine rm 0'" {
-  run machine rm 0
+  run machine rm 0 -y
   [ "$status" -eq 0 ]
 }
 
+@test "none: rm ask user confirmation when -y is not provided 'echo y | machine rm ba'" {
+  run machine create -d none --url none ba
+  [ "$status" -eq 0 ]
+  run bash -c "echo y | machine rm ba"
+  [ "$status" -eq 0 ]
+}
+
+@test "none: rm deny user confirmation when -y is not provided 'echo n | machine rm ab'" {
+  run machine create -d none --url none ab
+  [ "$status" -eq 0 ]
+  run bash -c "echo n | machine rm ab"
+  [ "$status" -eq 0 ]
+}
+
+@test "none: rm never prompt user confirmation with -f is provided 'echo n | machine rm -f ab'" {
+  run machine create -d none --url none c
+  [ "$status" -eq 0 ]
+  run bash -c "machine rm -f c"
+  [ "$status" -eq 0 ]
+  [[ ${lines[0]} == "Successfully removed c" ]]
+}
+
 # Should be replaced by the test below
 @test "none: rm is successful 'machine rm a'" {
-  run machine rm a
+  run machine rm a -y
   [ "$status" -eq 0 ]
 }
 
 @test "none: rm is case insensitive 'machine rm A'" {
   skip
-  run machine rm A
+  run machine rm A -y
   [ "$status" -eq 0 ]
 }
diff --git a/test/integration/cli/ls.bats b/test/integration/cli/ls.bats
@@ -9,7 +9,7 @@ setup () {
 }
 
 teardown () {
-  machine rm $(machine ls -q)
+  machine rm -y $(machine ls -q)
   echo_to_log
 }
 
diff --git a/test/integration/run-bats.sh b/test/integration/run-bats.sh
@@ -15,7 +15,7 @@ function quiet_run () {
 
 function cleanup_machines() {
     if [[ $(machine ls -q | wc -l) -ne 0 ]]; then
-        quiet_run machine rm -f $(machine ls -q)
+        quiet_run machine rm -y -f $(machine ls -q)
     fi
 }
 

Original file line number	Diff line number	Diff line change
`@@ -9,7 +9,7 @@ setup () {`
`9`	`9`	`}`
`10`	`10`
`11`	`11`	`teardown () {`
`12`		`- machine rm $(machine ls -q)`
	`12`	`+ machine rm -y $(machine ls -q)`
`13`	`13`	`echo_to_log`
`14`	`14`	`}`
`15`	`15`
Original file line number	Diff line number	Diff line change
`@@ -15,7 +15,7 @@ function quiet_run () {`
`15`	`15`
`16`	`16`	`function cleanup_machines() {`
`17`	`17`	`if [[ $(machine ls -q \| wc -l) -ne 0 ]]; then`
`18`		`- quiet_run machine rm -f $(machine ls -q)`
	`18`	`+ quiet_run machine rm -y -f $(machine ls -q)`
`19`	`19`	`fi`
`20`	`20`	`}`
`21`	`21`