Removed NAME from `volume ls` output, allow `volume delete <ID>` using IDs from `volume ls`.

Signed-off-by: Guillaume Tardif <guillaume.tardif@docker.com>

aci/volumes.go

@@ -19,6 +19,7 @@ package aci
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	"github.com/docker/compose-cli/progress"
 
@@ -80,17 +81,10 @@ type VolumeCreateOptions struct {
 	Fileshare string
 }
 
-//VolumeDeleteOptions options to create a new ACI volume
-type VolumeDeleteOptions struct {
-	Account       string
-	Fileshare     string
-	DeleteAccount bool
-}
-
 func (cs *aciVolumeService) Create(ctx context.Context, options interface{}) (volumes.Volume, error) {
 	opts, ok := options.(VolumeCreateOptions)
 	if !ok {
-		return volumes.Volume{}, errors.New("Could not read azure LoginParams struct from generic parameter")
+		return volumes.Volume{}, errors.New("Could not read azure VolumeCreateOptions struct from generic parameter")
 	}
 	w := progress.ContextWriter(ctx)
 	w.Event(event(opts.Account, progress.Working, "Validating"))
@@ -105,7 +99,6 @@ func (cs *aciVolumeService) Create(ctx context.Context, options interface{}) (vo
 		if account.StatusCode != 404 {
 			return volumes.Volume{}, err
 		}
-		//TODO confirm storage account creation
 		result, err := accountClient.CheckNameAvailability(ctx, storage.AccountCheckNameAvailabilityParameters{
 			Name: to.StringPtr(opts.Account),
 			Type: to.StringPtr("Microsoft.Storage/storageAccounts"),
@@ -177,49 +170,62 @@ func errorEvent(resource string) progress.Event {
 	}
 }
 
-func (cs *aciVolumeService) Delete(ctx context.Context, options interface{}) error {
+func (cs *aciVolumeService) Delete(ctx context.Context, id string, options interface{}) error {
-	opts, ok := options.(VolumeDeleteOptions)
+	tokens := strings.Split(id, "@")
-	if !ok {
+	if len(tokens) != 2 {
-		return errors.New("Could not read azure VolumeDeleteOptions struct from generic parameter")
+		return errors.New("wrong format for volume ID : should be storageaccount@fileshare")
-	}
-	if opts.DeleteAccount {
-		//TODO check if there are other fileshares on this account
-		storageAccountsClient, err := login.NewStorageAccountsClient(cs.aciContext.SubscriptionID)
-		if err != nil {
-			return err
-		}
-
-		result, err := storageAccountsClient.Delete(ctx, cs.aciContext.ResourceGroup, opts.Account)
-		if result.StatusCode == 204 {
-			return errors.Wrapf(errdefs.ErrNotFound, "storage account %s does not exist", opts.Account)
-		}
-
-		return err
 	}
+	storageAccount := tokens[0]
+	fileshare := tokens[1]
 
 	fileShareClient, err := login.NewFileShareClient(cs.aciContext.SubscriptionID)
 	if err != nil {
 		return err
 	}
-
+	fileShareItemsPage, err := fileShareClient.List(ctx, cs.aciContext.ResourceGroup, storageAccount, "", "", "")
-	result, err := fileShareClient.Delete(ctx, cs.aciContext.ResourceGroup, opts.Account, opts.Fileshare)
+	if err != nil {
-	if result.StatusCode == 204 {
+		return err
-		return errors.Wrapf(errdefs.ErrNotFound, "fileshare %s does not exist", opts.Fileshare)
 	}
-	if result.StatusCode == 404 {
+	fileshares := fileShareItemsPage.Values()
-		return errors.Wrapf(errdefs.ErrNotFound, "storage account %s does not exist", opts.Account)
+	if len(fileshares) == 1 && *fileshares[0].Name == fileshare {
+		storageAccountsClient, err := login.NewStorageAccountsClient(cs.aciContext.SubscriptionID)
+		if err != nil {
+			return err
+		}
+		account, err := storageAccountsClient.GetProperties(ctx, cs.aciContext.ResourceGroup, storageAccount, "")
+		if err != nil {
+			return err
+		}
+		if err == nil {
+			if _, ok := account.Tags[dockerVolumeTag]; ok {
+				result, err := storageAccountsClient.Delete(ctx, cs.aciContext.ResourceGroup, storageAccount)
+				if result.StatusCode == 204 {
+					return errors.Wrapf(errdefs.ErrNotFound, "storage account %s does not exist", storageAccount)
+				}
+				return err
+			}
+		}
+	}
+
+	result, err := fileShareClient.Delete(ctx, cs.aciContext.ResourceGroup, storageAccount, fileshare)
+	if result.StatusCode == 204 {
+		return errors.Wrapf(errdefs.ErrNotFound, "fileshare %s does not exist", fileshare)
 	}
 	return err
 }
 
 func toVolume(account storage.Account, fileShareName string) volumes.Volume {
 	return volumes.Volume{
-		ID:          fmt.Sprintf("%s@%s", *account.Name, fileShareName),
+		ID:          VolumeID(*account.Name, fileShareName),
-		Name:        fileShareName,
 		Description: fmt.Sprintf("Fileshare %s in %s storage account", fileShareName, *account.Name),
 	}
 }
 
+// VolumeID generate volume ID from azure storage accoun & fileshare
+func VolumeID(storageAccount string, fileShareName string) string {
+	return fmt.Sprintf("%s@%s", storageAccount, fileShareName)
+}
+
 func defaultStorageAccountParams(aciContext store.AciContext) storage.AccountCreateParameters {
 	tags := map[string]*string{dockerVolumeTag: to.StringPtr(dockerVolumeTag)}
 	return storage.AccountCreateParameters{

api/client/volume.go

@@ -34,6 +34,6 @@ func (c *volumeService) Create(ctx context.Context, options interface{}) (volume
 	return volumes.Volume{}, errdefs.ErrNotImplemented
 }
 
-func (c *volumeService) Delete(ctx context.Context, options interface{}) error {
+func (c *volumeService) Delete(ctx context.Context, id string, options interface{}) error {
 	return errdefs.ErrNotImplemented
 }

api/volumes/api.go

@@ -23,7 +23,6 @@ import (
 // Volume volume info
 type Volume struct {
 	ID          string
-	Name        string
 	Description string
 }
 
@@ -34,5 +33,5 @@ type Service interface {
 	// Create creates a new volume
 	Create(ctx context.Context, options interface{}) (Volume, error)
 	// Delete deletes an existing volume
-	Delete(ctx context.Context, options interface{}) error
+	Delete(ctx context.Context, volumeID string, options interface{}) error
 }

cli/cmd/volume/acilist.go

@@ -53,9 +53,9 @@ func listVolume() *cobra.Command {
 func printList(out io.Writer, volumes []volumes.Volume) {
 	printSection(out, func(w io.Writer) {
 		for _, vol := range volumes {
-			fmt.Fprintf(w, "%s\t%s\t%s\n", vol.ID, vol.Name, vol.Description) // nolint:errcheck
+			fmt.Fprintf(w, "%s\t%s\n", vol.ID, vol.Description) // nolint:errcheck
 		}
-	}, "ID", "NAME", "DESCRIPTION")
+	}, "ID", "DESCRIPTION")
 }
 
 func printSection(out io.Writer, printer func(io.Writer), headers ...string) {

cli/cmd/volume/acilist_test.go

@@ -29,7 +29,6 @@ func TestPrintList(t *testing.T) {
 	secrets := []volumes.Volume{
 		{
 			ID:          "volume@123",
-			Name:        "123",
 			Description: "volume 123",
 		},
 	}

cli/cmd/volume/acivolume.go

@@ -19,6 +19,9 @@ package volume
 import (
 	"context"
 	"fmt"
+	"strings"
+
+	"github.com/hashicorp/go-multierror"
 
 	"github.com/spf13/cobra"
 
@@ -64,8 +67,8 @@ func createVolume() *cobra.Command {
 			if err != nil {
 				return err
 			}
-			fmt.Printf("volume successfully created\n")
+			fmt.Println(aci.VolumeID(aciOpts.Account, aciOpts.Fileshare))
-			return err
+			return nil
 		},
 	}
 
@@ -75,22 +78,37 @@ func createVolume() *cobra.Command {
 }
 
 func rmVolume() *cobra.Command {
-	aciOpts := aci.VolumeDeleteOptions{}
 	cmd := &cobra.Command{
-		Use:   "rm",
+		Use:   "rm [OPTIONS] VOLUME [VOLUME...]",
-		Short: "Deletes an Azure file share and/or the Azure storage account.",
+		Short: "Remove one or more volumes.",
-		Args:  cobra.ExactArgs(0),
+		Args:  cobra.MinimumNArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			c, err := client.New(cmd.Context())
 			if err != nil {
 				return err
 			}
-			return c.VolumeService().Delete(cmd.Context(), aciOpts)
+			var errs *multierror.Error
+			for _, id := range args {
+				err = c.VolumeService().Delete(cmd.Context(), id, nil)
+				if err != nil {
+					errs = multierror.Append(errs, err)
+					continue
+				}
+				fmt.Println(id)
+			}
+			if errs != nil {
+				errs.ErrorFormat = formatErrors
+			}
+			return errs.ErrorOrNil()
 		},
 	}
-
-	cmd.Flags().StringVar(&aciOpts.Account, "storage-account", "", "Storage account name")
-	cmd.Flags().StringVar(&aciOpts.Fileshare, "fileshare", "", "Fileshare name")
-	cmd.Flags().BoolVar(&aciOpts.DeleteAccount, "delete-storage-account", false, "Also delete storage account")
 	return cmd
 }
+
+func formatErrors(errs []error) string {
+	messages := make([]string, len(errs))
+	for i, err := range errs {
+		messages[i] = "Error: " + err.Error()
+	}
+	return strings.Join(messages, "\n")
+}

cli/cmd/volume/testdata/volumes-out.golden

@@ -1,2 +1,2 @@
-ID                  NAME                DESCRIPTION
+ID                  DESCRIPTION
-volume@123          123                 volume 123
+volume@123          volume 123

tests/aci-e2e/e2e-aci_test.go

@@ -162,9 +162,10 @@ func TestContainerRunVolume(t *testing.T) {
 	t.Run("create volumes", func(t *testing.T) {
 		c.RunDockerCmd("volume", "create", "--storage-account", accountName, "--fileshare", fileshareName)
 	})
+	volumeID = accountName + "@" + fileshareName
 
 	t.Cleanup(func() {
-		c.RunDockerCmd("volume", "rm", "--storage-account", accountName, "--delete-storage-account")
+		c.RunDockerCmd("volume", "rm", volumeID)
 		res := c.RunDockerCmd("volume", "ls")
 		lines := lines(res.Stdout())
 		assert.Equal(t, len(lines), 1)
@@ -173,6 +174,7 @@ func TestContainerRunVolume(t *testing.T) {
 	t.Run("create second fileshare", func(t *testing.T) {
 		c.RunDockerCmd("volume", "create", "--storage-account", accountName, "--fileshare", "dockertestshare2")
 	})
+	volumeID2 := accountName + "@dockertestshare2"
 
 	t.Run("list volumes", func(t *testing.T) {
 		res := c.RunDockerCmd("volume", "ls")
@@ -180,18 +182,14 @@ func TestContainerRunVolume(t *testing.T) {
 		assert.Equal(t, len(lines), 3)
 		firstAccount := lines[1]
 		fields := strings.Fields(firstAccount)
-		volumeID = accountName + "@" + fileshareName
 		assert.Equal(t, fields[0], volumeID)
-		assert.Equal(t, fields[1], fileshareName)
 		secondAccount := lines[2]
 		fields = strings.Fields(secondAccount)
-		assert.Equal(t, fields[0], accountName+"@dockertestshare2")
+		assert.Equal(t, fields[0], volumeID2)
-		assert.Equal(t, fields[1], "dockertestshare2")
-		//assert.Assert(t, fields[2], strings.Contains(firstAccount, fmt.Sprintf("Fileshare %s in %s storage account", fileshareName, accountName)))
 	})
 
 	t.Run("delete only fileshare", func(t *testing.T) {
-		c.RunDockerCmd("volume", "rm", "--storage-account", accountName, "--fileshare", "dockertestshare2")
+		c.RunDockerCmd("volume", "rm", volumeID2)
 		res := c.RunDockerCmd("volume", "ls")
 		lines := lines(res.Stdout())
 		assert.Equal(t, len(lines), 2)