Merge pull request #7007 from axw/cmd-storage-list-combined

cmd/juju/storage: list volumes/filesystems too ## Description of change When running "juju storage", show volume and filesystem details alongside storage instances. The plan is to normalise the commands relating to storage, so we end up with: - juju attach-storage <unit> store=<volume|filesystem> - juju detach-storage <storage-instance|volume|filesystem> - juju remove-storage <storage-instance|volume|filesystem> ## QA steps 1. juju bootstrap whatever 2. juju deploy postgresql --storage pgdata=1G 3. juju storage (observe info about storage, filesystem, volume all shown) 4. juju storage --filesystem (observe info about only filesystem shown) 5. juju storage --volume (observe info about only volume shown) ## Documentation changes Yes, but backwards compatible. Any sample storage listing output in the docs should be updated. ## Bug reference None.
juju · Feb 21, 2017 · 428767d · 428767d
2 parents 262255b + 26eb75d
commit 428767d
Show file tree

Hide file tree

Showing 9 changed files with 309 additions and 85 deletions.
diff --git a/cmd/juju/storage/filesystem.go b/cmd/juju/storage/filesystem.go
@@ -52,9 +52,9 @@ type MachineFilesystemAttachment struct {
 }
 
 // generateListFilesystemOutput returns a map filesystem IDs to filesystem info
-func (c *listCommand) generateListFilesystemsOutput(ctx *cmd.Context, api StorageListAPI) (output interface{}, err error) {
+func generateListFilesystemsOutput(ctx *cmd.Context, api StorageListAPI, ids []string) (map[string]FilesystemInfo, error) {
 
-	results, err := api.ListFilesystems(c.ids)
+	results, err := api.ListFilesystems(ids)
 	if err != nil {
 		return nil, err
 	}
@@ -72,18 +72,7 @@ func (c *listCommand) generateListFilesystemsOutput(ctx *cmd.Context, api Storag
 	if len(valid) == 0 {
 		return nil, nil
 	}
-	info, err := convertToFilesystemInfo(valid)
-	if err != nil {
-		return nil, err
-	}
-	switch c.out.Name() {
-	case "yaml", "json":
-		output = map[string]map[string]FilesystemInfo{"filesystems": info}
-	default:
-		output = info
-	}
-
-	return output, nil
+	return convertToFilesystemInfo(valid)
 }
 
 // convertToFilesystemInfo returns a map of filesystem IDs to filesystem info.

diff --git a/cmd/juju/storage/filesystemlist_test.go b/cmd/juju/storage/filesystemlist_test.go
@@ -88,6 +88,7 @@ func (s *ListSuite) TestFilesystemListWithErrorResults(c *gc.C) {
 }
 
 var expectedFilesystemListTabular = `
+[Filesystems]
 Machine  Unit         Storage      Id   Volume  Provider id                       Mountpoint  Size    State      Message
 0        abc/0        db-dir/1001  0/0  0/1     provider-supplied-filesystem-0-0  /mnt/fuji   512MiB  attached   
 0        transcode/0  shared-fs/0  4            provider-supplied-filesystem-4    /mnt/doom   1.0GiB  attached   

diff --git a/cmd/juju/storage/filesystemlistformatters.go b/cmd/juju/storage/filesystemlistformatters.go
@@ -20,6 +20,7 @@ func formatFilesystemListTabular(writer io.Writer, infos map[string]FilesystemIn
 	print := func(values ...string) {
 		fmt.Fprintln(tw, strings.Join(values, "\t"))
 	}
+	print("[Filesystems]")
 	print("Machine", "Unit", "Storage", "Id", "Volume", "Provider id", "Mountpoint", "Size", "State", "Message")
 
 	filesystemAttachmentInfos := make(filesystemAttachmentInfos, 0, len(infos))

diff --git a/cmd/juju/storage/list.go b/cmd/juju/storage/list.go
@@ -4,6 +4,7 @@
 package storage
 
 import (
+	"fmt"
 	"io"
 
 	"github.com/juju/cmd"
@@ -24,7 +25,7 @@ func NewListCommand() cmd.Command {
 }
 
 const listCommandDoc = `
-List information about storage instances.
+List information about storage.
 `
 
 // listCommand returns storage instances.
@@ -37,12 +38,6 @@ type listCommand struct {
 	newAPIFunc func() (StorageListAPI, error)
 }
 
-// Init implements Command.Init.
-func (c *listCommand) Init(args []string) (err error) {
-	c.ids = args
-	return nil
-}
-
 // Info implements Command.Info.
 func (c *listCommand) Info() *cmd.Info {
 	return &cmd.Info{
@@ -62,10 +57,24 @@ func (c *listCommand) SetFlags(f *gnuflag.FlagSet) {
 		"json":    cmd.FormatJson,
 		"tabular": formatListTabular,
 	})
+	// TODO(axw) deprecate these flags, and introduce separate commands
+	// for listing just filesystems or volumes.
 	f.BoolVar(&c.filesystem, "filesystem", false, "List filesystem storage")
 	f.BoolVar(&c.volume, "volume", false, "List volume storage")
 }
 
+// Init implements Command.Init.
+func (c *listCommand) Init(args []string) (err error) {
+	if c.filesystem && c.volume {
+		return errors.New("--filesystem and --volume can not be used together")
+	}
+	if len(args) > 0 && !c.filesystem && !c.volume {
+		return errors.New("specifying IDs only supported with --filesystem and --volume flags")
+	}
+	c.ids = args
+	return nil
+}
+
 // Run implements Command.Run.
 func (c *listCommand) Run(ctx *cmd.Context) (err error) {
 	api, err := c.newAPIFunc()
@@ -74,22 +83,47 @@ func (c *listCommand) Run(ctx *cmd.Context) (err error) {
 	}
 	defer api.Close()
 
-	var output interface{}
-	if c.filesystem {
-		output, err = c.generateListFilesystemsOutput(ctx, api)
-	} else if c.volume {
-		output, err = c.generateListVolumeOutput(ctx, api)
-	} else {
-		output, err = c.generateListOutput(ctx, api)
+	var wantStorage, wantVolumes, wantFilesystems bool
+	switch {
+	case c.filesystem:
+		wantFilesystems = true
+	case c.volume:
+		wantVolumes = true
+	default:
+		wantStorage = true
+		wantVolumes = true
+		wantFilesystems = true
 	}
-	if err != nil {
-		return err
+
+	var combined combinedStorage
+	if wantFilesystems {
+		filesystems, err := generateListFilesystemsOutput(ctx, api, c.ids)
+		if err != nil {
+			return err
+		}
+		combined.Filesystems = filesystems
 	}
-	if output == nil && c.out.Name() == "tabular" {
-		ctx.Infof("No storage to display.")
+	if wantVolumes {
+		volumes, err := generateListVolumeOutput(ctx, api, c.ids)
+		if err != nil {
+			return err
+		}
+		combined.Volumes = volumes
+	}
+	if wantStorage {
+		storageInstances, err := generateListStorageOutput(ctx, api)
+		if err != nil {
+			return err
+		}
+		combined.StorageInstances = storageInstances
+	}
+	if combined.empty() {
+		if c.out.Name() == "tabular" {
+			ctx.Infof("No storage to display.")
+		}
 		return nil
 	}
-	return c.out.Write(ctx, output)
+	return c.out.Write(ctx, combined)
 }
 
 // StorageAPI defines the API methods that the storage commands use.
@@ -100,41 +134,53 @@ type StorageListAPI interface {
 	ListVolumes(machines []string) ([]params.VolumeDetailsListResult, error)
 }
 
-// generateListOutput returns a map of storage details
-func (c *listCommand) generateListOutput(ctx *cmd.Context, api StorageListAPI) (output interface{}, err error) {
-
+// generateListStorageOutput returns a map of storage details
+func generateListStorageOutput(ctx *cmd.Context, api StorageListAPI) (map[string]StorageInfo, error) {
 	results, err := api.ListStorageDetails()
 	if err != nil {
 		return nil, err
 	}
 	if len(results) == 0 {
 		return nil, nil
 	}
-	details, err := formatStorageDetails(results)
-	if err != nil {
-		return nil, err
-	}
-	switch c.out.Name() {
-	case "yaml", "json":
-		output = map[string]map[string]StorageInfo{"storage": details}
-	default:
-		output = details
-	}
-	return output, nil
+	return formatStorageDetails(results)
 }
 
-func formatListTabular(writer io.Writer, value interface{}) error {
-	switch value := value.(type) {
-	case map[string]StorageInfo:
-		return formatStorageListTabular(writer, value)
-
-	case map[string]FilesystemInfo:
-		return formatFilesystemListTabular(writer, value)
+type combinedStorage struct {
+	StorageInstances map[string]StorageInfo    `yaml:"storage,omitempty" json:"storage,omitempty"`
+	Filesystems      map[string]FilesystemInfo `yaml:"filesystems,omitempty" json:"filesystems,omitempty"`
+	Volumes          map[string]VolumeInfo     `yaml:"volumes,omitempty" json:"volumes,omitempty"`
+}
 
-	case map[string]VolumeInfo:
-		return formatVolumeListTabular(writer, value)
+func (c *combinedStorage) empty() bool {
+	return len(c.StorageInstances) == 0 && len(c.Filesystems) == 0 && len(c.Volumes) == 0
+}
 
-	default:
-		return errors.Errorf("unexpected value of type %T", value)
+func formatListTabular(writer io.Writer, value interface{}) error {
+	combined := value.(combinedStorage)
+	var newline bool
+	if len(combined.StorageInstances) > 0 {
+		if err := formatStorageListTabular(writer, combined.StorageInstances); err != nil {
+			return err
+		}
+		newline = true
+	}
+	if len(combined.Filesystems) > 0 {
+		if newline {
+			fmt.Fprintln(writer)
+		}
+		if err := formatFilesystemListTabular(writer, combined.Filesystems); err != nil {
+			return err
+		}
+		newline = true
 	}
+	if len(combined.Volumes) > 0 {
+		if newline {
+			fmt.Fprintln(writer)
+		}
+		if err := formatVolumeListTabular(writer, combined.Volumes); err != nil {
+			return err
+		}
+	}
+	return nil
 }