Merge pull request #2738 from jeanlaurent/better-remove

Enhance remove

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()
+}

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"))
 }

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