diff --git a/commands/rm.go b/commands/rm.go index cb64430ecd..bb80334fff 100644 --- a/commands/rm.go +++ b/commands/rm.go @@ -3,6 +3,10 @@ package commands import ( "fmt" + "strings" + + "errors" + "github.com/docker/machine/libmachine" "github.com/docker/machine/libmachine/log" ) @@ -13,41 +17,56 @@ func cmdRm(c CommandLine, api libmachine.API) error { return ErrNoMachineSpecified } + log.Info(fmt.Sprintf("About to remove %s", strings.Join(c.Args(), ","))) + force := c.Bool("force") confirm := c.Bool("y") + var errorOccured string - for _, hostName := range c.Args() { - h, loaderr := api.Load(hostName) - if loaderr != nil { - // On --force, continue to remove on-disk files/dir - if !force { - return fmt.Errorf("Error removing host %q: %s", hostName, loaderr) - } - log.Errorf("Error removing host %q: %s. Continuing on `-f`, host instance may by running", hostName, loaderr) - } - - if !confirm && !force { - userinput, err := confirmInput(fmt.Sprintf("Do you really want to remove %q?", hostName)) - if !userinput { - return err - } - } - - if loaderr == nil { - if err := h.Driver.Remove(); err != nil { - if !force { - log.Errorf("Provider error removing machine %q: %s", hostName, err) - continue - } - } - } - - if err := api.Remove(hostName); err != nil { - log.Errorf("Error removing machine %q from store: %s", hostName, err) - } else { - log.Infof("Successfully removed %s", hostName) - } + if !userConfirm(confirm, force) { + return nil } + for _, hostName := range c.Args() { + err := removeRemoteMachine(hostName, api) + if err != nil { + errorOccured = fmt.Sprintf("Error removing host %q: %s", hostName, err) + log.Errorf(errorOccured) + } + + if err == nil || force { + removeErr := api.Remove(hostName) + if removeErr != nil { + log.Errorf("Error removing machine %q from store: %s", hostName, removeErr) + } else { + log.Infof("Successfully removed %s", hostName) + } + } + } + if errorOccured != "" { + return errors.New(errorOccured) + } return nil } + +func userConfirm(confirm bool, force bool) bool { + if confirm || force { + return true + } + + sure, err := confirmInput(fmt.Sprintf("Are you sure?")) + if err != nil { + return false + } + + return sure +} + +func removeRemoteMachine(hostName string, api libmachine.API) error { + currentHost, loaderr := api.Load(hostName) + if loaderr != nil { + return loaderr + } + + return currentHost.Driver.Remove() +} diff --git a/commands/rm_test.go b/commands/rm_test.go index 5e41f0c4b6..2fda5f3114 100644 --- a/commands/rm_test.go +++ b/commands/rm_test.go @@ -3,6 +3,8 @@ package commands import ( "testing" + "errors" + "github.com/docker/machine/commands/commandstest" "github.com/docker/machine/drivers/fakedriver" "github.com/docker/machine/libmachine/host" @@ -133,7 +135,99 @@ func TestCmdRmforceConfirmUnset(t *testing.T) { } err := cmdRm(commandLine, api) - assert.EqualError(t, err, "EOF") + assert.NoError(t, err) + + assert.True(t, libmachinetest.Exists(api, "machineToRemove1")) +} + +type DriverWithRemoveWhichFail struct { + fakedriver.Driver +} + +func (d *DriverWithRemoveWhichFail) Remove() error { + return errors.New("unknown error") +} + +func TestDontStopWhenADriverRemovalFails(t *testing.T) { + commandLine := &commandstest.FakeCommandLine{ + CliArgs: []string{"machineToRemove1", "machineToRemove2", "machineToRemove3"}, + LocalFlags: &commandstest.FakeFlagger{ + Data: map[string]interface{}{ + "y": true, + }, + }, + } + api := &libmachinetest.FakeAPI{ + Hosts: []*host.Host{ + { + Name: "machineToRemove1", + Driver: &fakedriver.Driver{}, + }, + { + Name: "machineToRemove2", + Driver: &DriverWithRemoveWhichFail{}, + }, + { + Name: "machineToRemove3", + Driver: &fakedriver.Driver{}, + }, + }, + } + + err := cmdRm(commandLine, api) + assert.EqualError(t, err, "Error removing host \"machineToRemove2\": unknown error") + + assert.False(t, libmachinetest.Exists(api, "machineToRemove1")) + assert.True(t, libmachinetest.Exists(api, "machineToRemove2")) + assert.False(t, libmachinetest.Exists(api, "machineToRemove3")) +} + +func TestForceRemoveEvenWhenItFails(t *testing.T) { + commandLine := &commandstest.FakeCommandLine{ + CliArgs: []string{"machineToRemove1"}, + LocalFlags: &commandstest.FakeFlagger{ + Data: map[string]interface{}{ + "y": true, + "force": true, + }, + }, + } + api := &libmachinetest.FakeAPI{ + Hosts: []*host.Host{ + { + Name: "machineToRemove1", + Driver: &DriverWithRemoveWhichFail{}, + }, + }, + } + + err := cmdRm(commandLine, api) + assert.EqualError(t, err, "Error removing host \"machineToRemove1\": unknown error") + + assert.False(t, libmachinetest.Exists(api, "machineToRemove1")) +} + +func TestDontRemoveMachineIsRemovalFailsAndNotForced(t *testing.T) { + commandLine := &commandstest.FakeCommandLine{ + CliArgs: []string{"machineToRemove1"}, + LocalFlags: &commandstest.FakeFlagger{ + Data: map[string]interface{}{ + "y": true, + "force": false, + }, + }, + } + api := &libmachinetest.FakeAPI{ + Hosts: []*host.Host{ + { + Name: "machineToRemove1", + Driver: &DriverWithRemoveWhichFail{}, + }, + }, + } + + err := cmdRm(commandLine, api) + assert.EqualError(t, err, "Error removing host \"machineToRemove1\": unknown error") assert.True(t, libmachinetest.Exists(api, "machineToRemove1")) } diff --git a/test/integration/cli/create-rm.bats b/test/integration/cli/create-rm.bats index fd8158b24b..ecac1403ea 100644 --- a/test/integration/cli/create-rm.bats +++ b/test/integration/cli/create-rm.bats @@ -73,7 +73,7 @@ load ${BASE_TEST_DIR}/helpers.bash @test "none: rm non existent machine fails 'machine rm ∞'" { run machine rm ∞ -y [ "$status" -eq 1 ] - [[ ${lines[0]} == "Error removing host \"∞\": Host does not exist: \"∞\"" ]] + [[ ${lines[1]} == "Error removing host \"∞\": Host does not exist: \"∞\"" ]] } @test "none: rm is successful 'machine rm 0'" { @@ -100,7 +100,7 @@ load ${BASE_TEST_DIR}/helpers.bash [ "$status" -eq 0 ] run bash -c "machine rm -f c" [ "$status" -eq 0 ] - [[ ${lines[0]} == "Successfully removed c" ]] + [[ ${lines[1]} == "Successfully removed c" ]] } # Should be replaced by the test below