From 77fba72d13edd1b7964c2dbeeedbe190f1831ae8 Mon Sep 17 00:00:00 2001 From: Anil Belur Date: Tue, 24 Nov 2015 21:34:33 +0530 Subject: [PATCH] Fixes #2349 - rm get user confirmation before proceeding further * adds new flag `-y` prompting for user confirmation before removal * Modified existing integration tests to work with the fix #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 --- commands/commands.go | 4 ++++ commands/rm.go | 8 ++++++++ commands/rm_test.go | 5 +++++ docs/get-started-cloud.md | 5 +++++ docs/reference/rm.md | 4 ++++ test/integration/cli/create-rm.bats | 32 ++++++++++++++++++++++++----- test/integration/cli/ls.bats | 2 +- test/integration/run-bats.sh | 2 +- 8 files changed, 55 insertions(+), 7 deletions(-) diff --git a/commands/commands.go b/commands/commands.go index 32a3a6534b..839e13a816 100644 --- 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 index 9e67434f83..cc50df1bbc 100644 --- a/commands/rm.go +++ b/commands/rm.go @@ -14,6 +14,7 @@ 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) @@ -21,6 +22,13 @@ func cmdRm(c CommandLine, api libmachine.API) error { 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 index f2e9391239..b90e8152ce 100644 --- 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 index 40ebcef09a..5a0b05683a 100644 --- 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 index c9a68c03e8..c8202fe2fb 100644 --- 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 index 8bd8e12525..fd8158b24b 100644 --- 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 index 4a76cddc7c..e1e2eb81c6 100644 --- 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 index f3390f3ce5..05970a4cc6 100755 --- 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 }