From 80d23a60977befc11817980a764ce0f4aed8d258 Mon Sep 17 00:00:00 2001 From: Guillaume Tardif Date: Wed, 9 Sep 2020 15:49:43 +0200 Subject: [PATCH] Removed NAME from `volume ls` output, allow `volume delete ` using IDs from `volume ls`. Signed-off-by: Guillaume Tardif --- aci/volumes.go | 76 ++++++++++++---------- api/client/volume.go | 2 +- api/volumes/api.go | 3 +- cli/cmd/volume/acilist.go | 4 +- cli/cmd/volume/acilist_test.go | 1 - cli/cmd/volume/acivolume.go | 40 ++++++++---- cli/cmd/volume/testdata/volumes-out.golden | 4 +- tests/aci-e2e/e2e-aci_test.go | 12 ++-- 8 files changed, 81 insertions(+), 61 deletions(-) diff --git a/aci/volumes.go b/aci/volumes.go index df7ab1082..3a43d68af 100644 --- a/aci/volumes.go +++ b/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 { - opts, ok := options.(VolumeDeleteOptions) - if !ok { - return errors.New("Could not read azure VolumeDeleteOptions struct from generic parameter") - } - 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 +func (cs *aciVolumeService) Delete(ctx context.Context, id string, options interface{}) error { + tokens := strings.Split(id, "@") + if len(tokens) != 2 { + return errors.New("wrong format for volume ID : should be storageaccount@fileshare") } + storageAccount := tokens[0] + fileshare := tokens[1] fileShareClient, err := login.NewFileShareClient(cs.aciContext.SubscriptionID) if err != nil { return err } - - result, err := fileShareClient.Delete(ctx, cs.aciContext.ResourceGroup, opts.Account, opts.Fileshare) - if result.StatusCode == 204 { - return errors.Wrapf(errdefs.ErrNotFound, "fileshare %s does not exist", opts.Fileshare) + fileShareItemsPage, err := fileShareClient.List(ctx, cs.aciContext.ResourceGroup, storageAccount, "", "", "") + if err != nil { + return err } - if result.StatusCode == 404 { - return errors.Wrapf(errdefs.ErrNotFound, "storage account %s does not exist", opts.Account) + fileshares := fileShareItemsPage.Values() + 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), - Name: fileShareName, + ID: VolumeID(*account.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{ diff --git a/api/client/volume.go b/api/client/volume.go index ea0ee378f..58c89ed8a 100644 --- a/api/client/volume.go +++ b/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 } diff --git a/api/volumes/api.go b/api/volumes/api.go index 29562f7ec..a753fcea9 100644 --- a/api/volumes/api.go +++ b/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 } diff --git a/cli/cmd/volume/acilist.go b/cli/cmd/volume/acilist.go index 3158cce94..7261f3427 100644 --- a/cli/cmd/volume/acilist.go +++ b/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) { diff --git a/cli/cmd/volume/acilist_test.go b/cli/cmd/volume/acilist_test.go index 3096ef2a2..a6a1e73d0 100644 --- a/cli/cmd/volume/acilist_test.go +++ b/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", }, } diff --git a/cli/cmd/volume/acivolume.go b/cli/cmd/volume/acivolume.go index de6c0a46b..a4e4dd6d6 100644 --- a/cli/cmd/volume/acivolume.go +++ b/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") - return err + fmt.Println(aci.VolumeID(aciOpts.Account, aciOpts.Fileshare)) + return nil }, } @@ -75,22 +78,37 @@ func createVolume() *cobra.Command { } func rmVolume() *cobra.Command { - aciOpts := aci.VolumeDeleteOptions{} cmd := &cobra.Command{ - Use: "rm", - Short: "Deletes an Azure file share and/or the Azure storage account.", - Args: cobra.ExactArgs(0), + Use: "rm [OPTIONS] VOLUME [VOLUME...]", + Short: "Remove one or more volumes.", + 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") +} diff --git a/cli/cmd/volume/testdata/volumes-out.golden b/cli/cmd/volume/testdata/volumes-out.golden index ed27e4491..9a4039ece 100644 --- a/cli/cmd/volume/testdata/volumes-out.golden +++ b/cli/cmd/volume/testdata/volumes-out.golden @@ -1,2 +1,2 @@ -ID NAME DESCRIPTION -volume@123 123 volume 123 +ID DESCRIPTION +volume@123 volume 123 diff --git a/tests/aci-e2e/e2e-aci_test.go b/tests/aci-e2e/e2e-aci_test.go index cb3d1f091..bd6911b8d 100644 --- a/tests/aci-e2e/e2e-aci_test.go +++ b/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[1], "dockertestshare2") - //assert.Assert(t, fields[2], strings.Contains(firstAccount, fmt.Sprintf("Fileshare %s in %s storage account", fileshareName, accountName))) + assert.Equal(t, fields[0], volumeID2) }) 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)