Added json output for storage create (#1404)

* Added json output for storage create

* Fixed unit tests

* Moved path to status in storage types.go

Signed-off-by: mik-dass <mrinald7@gmail.com>

* Rebased the PR

Signed-off-by: mik-dass <mrinald7@gmail.com>

* Removed status from StorageStatus struct

pkg/odo/cli/storage/create.go

@@ -49,14 +49,27 @@ func (o *StorageCreateOptions) Complete(name string, cmd *cobra.Command, args []
 
 // Validate validates the StorageCreateOptions based on completed values
 func (o *StorageCreateOptions) Validate() (err error) {
+	// check the machine readable format
+	if !util.CheckOutputFlag(o.OutputFlag) {
+		return fmt.Errorf("given output format %s is not supported", o.OutputFlag)
+	}
 	// validate storage path
 	return validateStoragePath(o.Client, o.storageName, o.Component(), o.Application)
 }
 
 // Run contains the logic for the odo storage create command
 func (o *StorageCreateOptions) Run() (err error) {
-	_, err = storage.Create(o.Client, o.storageName, o.storageSize, o.storagePath, o.Component(), o.Application)
-	if err == nil {
+	storageResult, err := storage.Create(o.Client, o.storageName, o.storageSize, o.storagePath, o.Component(), o.Application)
+	if err != nil {
+		return err
+	}
+	out, err := util.MachineOutput(o.OutputFlag, storageResult)
+	if err != nil {
+		return err
+	}
+	if out != "" {
+		fmt.Println(out)
+	} else {
 		log.Successf("Added storage %v to %v", o.storageName, o.Component())
 	}
 	return
@@ -85,5 +98,7 @@ func NewCmdStorageCreate(name, fullName string) *cobra.Command {
 	appCmd.AddApplicationFlag(storageCreateCmd)
 	componentCmd.AddComponentFlag(storageCreateCmd)
 
+	genericclioptions.AddOutputFlag(storageCreateCmd)
+
 	return storageCreateCmd
 }

pkg/odo/cli/storage/list.go

@@ -143,10 +143,9 @@ func getMachineReadableFormat(mounted bool, stor storage.Storage) storage.Storag
 		},
 		Spec: storage.StorageSpec{
 			Size: stor.Spec.Size,
-			Path: stor.Spec.Path,
 		},
 		Status: storage.StorageStatus{
-			Mounted: mounted,
+			Path: stor.Status.Path,
 		},
 	}
 
@@ -191,7 +190,7 @@ func printMountedStorageInComponent(client *occlient.Client, componentName strin
 	// iterating over all mounted storage and put in the mount storage table
 	if len(storageListMounted.Items) > 0 {
 		for _, mStorage := range storageListMounted.Items {
-			fmt.Fprintln(tabWriterMounted, mStorage.Name, "\t", mStorage.Spec.Size, "\t", mStorage.Spec.Path)
+			fmt.Fprintln(tabWriterMounted, mStorage.Name, "\t", mStorage.Spec.Size, "\t", mStorage.Status.Path)
 		}
 
 		// print all mounted storage of the given component

pkg/odo/cli/storage/storage.go

@@ -58,7 +58,7 @@ func validateStoragePath(client *occlient.Client, storagePath, componentName, ap
 		return err
 	}
 	for _, store := range storeList.Items {
-		if store.Spec.Path == storagePath {
+		if store.Status.Path == storagePath {
 			return errors.Errorf("there already is a storage mounted at %s", storagePath)
 		}
 	}

pkg/odo/util/cmdutils.go

@@ -88,7 +88,7 @@ func PrintComponentInfo(client *occlient.Client, currentComponentName string, co
 		LogErrorAndExit(err, "")
 		for _, storage := range componentDesc.Spec.Storage {
 			store := storages.Get(storage)
-			fmt.Printf(" - %v of size %v mounted to %v\n", store.Name, store.Spec.Size, store.Spec.Path)
+			fmt.Printf(" - %v of size %v mounted to %v\n", store.Name, store.Spec.Size, store.Status.Path)
 		}
 	}
 	// URL

pkg/storage/storage.go

@@ -28,14 +28,14 @@ func (storages StorageList) Get(storageName string) Storage {
 }
 
 // Create adds storage to given component of given application
-func Create(client *occlient.Client, name string, size string, path string, componentName string, applicationName string) (string, error) {
+func Create(client *occlient.Client, name string, size string, path string, componentName string, applicationName string) (Storage, error) {
 
 	// Namespace the component
 	// We will use name+applicationName instead of componentName+applicationName until:
 	// https://github.com/redhat-developer/odo/issues/504 is resolved.
 	namespacedOpenShiftObject, err := util.NamespaceOpenShiftObject(name, applicationName)
 	if err != nil {
-		return "", errors.Wrapf(err, "unable to create namespaced name")
+		return Storage{}, errors.Wrapf(err, "unable to create namespaced name")
 	}
 
 	labels := storagelabels.GetLabels(name, componentName, applicationName, true)
@@ -45,7 +45,7 @@ func Create(client *occlient.Client, name string, size string, path string, comp
 	// Create PVC
 	pvc, err := client.CreatePVC(generatePVCNameFromStorageName(namespacedOpenShiftObject), size, labels)
 	if err != nil {
-		return "", errors.Wrap(err, "unable to create PVC")
+		return Storage{}, errors.Wrap(err, "unable to create PVC")
 	}
 
 	// Get DeploymentConfig for the given component
@@ -53,16 +53,17 @@ func Create(client *occlient.Client, name string, size string, path string, comp
 	componentSelector := util.ConvertLabelsToSelector(componentLabels)
 	dc, err := client.GetOneDeploymentConfigFromSelector(componentSelector)
 	if err != nil {
-		return "", errors.Wrapf(err, "unable to get Deployment Config for component: %v in application: %v", componentName, applicationName)
+		return Storage{}, errors.Wrapf(err, "unable to get Deployment Config for component: %v in application: %v", componentName, applicationName)
 	}
 	glog.V(4).Infof("Deployment Config: %v is associated with the component: %v", dc.Name, componentName)
 
 	// Add PVC to DeploymentConfig
 	if err := client.AddPVCToDeploymentConfig(dc, pvc.Name, path); err != nil {
-		return "", errors.Wrap(err, "unable to add PVC to DeploymentConfig")
+		return Storage{}, errors.Wrap(err, "unable to add PVC to DeploymentConfig")
 	}
 
-	return dc.Name, nil
+	// getting the machine readable output format and mark status as active
+	return getMachineReadableFormat(*pvc, path), nil
 }
 
 // Unmount unmounts the given storage from the given component
@@ -198,7 +199,7 @@ func ListMounted(client *occlient.Client, componentName string, applicationName
 	}
 	var storageListMounted []Storage
 	for _, storage := range storageList.Items {
-		if storage.Spec.Path != "" {
+		if storage.Status.Path != "" {
 			storageListMounted = append(storageListMounted, storage)
 		}
 	}
@@ -307,7 +308,7 @@ func IsMounted(client *occlient.Client, storageName string, componentName string
 	}
 	for _, storage := range storageList.Items {
 		if storage.Name == storageName {
-			if storage.Spec.Path != "" {
+			if storage.Status.Path != "" {
 				return true, nil
 			}
 		}
@@ -321,7 +322,7 @@ func GetMountPath(client *occlient.Client, storageName string, componentName str
 	storageList, _ := List(client, componentName, applicationName)
 	for _, storage := range storageList.Items {
 		if storage.Name == storageName {
-			mPath = storage.Spec.Path
+			mPath = storage.Status.Path
 		}
 	}
 	return mPath
@@ -375,7 +376,7 @@ func GetStorageNameFromMountPath(client *occlient.Client, path string, component
 		return "", errors.Wrapf(err, "unable to list storage for component %v", componentName)
 	}
 	for _, storage := range storageList.Items {
-		if storage.Spec.Path == path {
+		if storage.Status.Path == path {
 			return storage.Name, nil
 		}
 	}
@@ -395,6 +396,7 @@ func getMachineReadableFormatForList(storage []Storage) StorageList {
 }
 
 // getMachineReadableFormat gives machine readable Storage definition
+// storagePath indicates the path to which the storage is mounted to, "" if not mounted
 func getMachineReadableFormat(pvc corev1.PersistentVolumeClaim, storagePath string) Storage {
 	storageName := getStorageFromPVC(&pvc)
 	storageSize := pvc.Spec.Resources.Requests[corev1.ResourceStorage]
@@ -402,9 +404,11 @@ func getMachineReadableFormat(pvc corev1.PersistentVolumeClaim, storagePath stri
 		TypeMeta:   metav1.TypeMeta{Kind: "storage", APIVersion: "odo.openshift.io/v1alpha1"},
 		ObjectMeta: metav1.ObjectMeta{Name: storageName},
 		Spec: StorageSpec{
-			Path: storagePath,
 			Size: storageSize.String(),
 		},
+		Status: StorageStatus{
+			Path: storagePath,
+		},
 	}
 
 }

pkg/storage/storage_test.go

@@ -61,13 +61,14 @@ func Test_getMachineReadableFormat(t *testing.T) {
 		t.Errorf("unable to parse size")
 	}
 	tests := []struct {
-		name        string
-		inputPVC    *corev1.PersistentVolumeClaim
-		mountedPath string
-		want        Storage
+		name         string
+		inputPVC     *corev1.PersistentVolumeClaim
+		mountedPath  string
+		activeStatus bool
+		want         Storage
 	}{
 		{
-			name: "test case 1: with a pvc",
+			name: "test case 1: with a pvc, valid path and mounted status",
 			inputPVC: &corev1.PersistentVolumeClaim{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "pvc-example",
@@ -83,16 +84,51 @@ func Test_getMachineReadableFormat(t *testing.T) {
 					},
 				},
 			},
-			mountedPath: "data",
+			mountedPath:  "data",
+			activeStatus: true,
 			want: Storage{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "pvc-example",
 				},
 				TypeMeta: metav1.TypeMeta{Kind: "storage", APIVersion: "odo.openshift.io/v1alpha1"},
 				Spec: StorageSpec{
-					Path: "data",
 					Size: "100Mi",
 				},
+				Status: StorageStatus{
+					Path: "data",
+				},
+			},
+		},
+		{
+			name: "test case 2: with a pvc, empty path and unmounted status",
+			inputPVC: &corev1.PersistentVolumeClaim{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "pvc-example",
+					Labels: map[string]string{
+						storagelabels.StorageLabel: "pvc-example",
+					},
+				},
+				Spec: corev1.PersistentVolumeClaimSpec{
+					Resources: corev1.ResourceRequirements{
+						Requests: corev1.ResourceList{
+							corev1.ResourceStorage: quantity,
+						},
+					},
+				},
+			},
+			mountedPath:  "",
+			activeStatus: false,
+			want: Storage{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "pvc-example",
+				},
+				TypeMeta: metav1.TypeMeta{Kind: "storage", APIVersion: "odo.openshift.io/v1alpha1"},
+				Spec: StorageSpec{
+					Size: "100Mi",
+				},
+				Status: StorageStatus{
+					Path: "",
+				},
 			},
 		},
 	}
@@ -126,6 +162,8 @@ func Test_getMachineReadableFormatForList(t *testing.T) {
 					},
 					Spec: StorageSpec{
 						Size: "100Mi",
+					},
+					Status: StorageStatus{
 						Path: "data",
 					},
 				},
@@ -147,6 +185,8 @@ func Test_getMachineReadableFormatForList(t *testing.T) {
 						},
 						Spec: StorageSpec{
 							Size: "100Mi",
+						},
+						Status: StorageStatus{
 							Path: "data",
 						},
 					},
@@ -166,6 +206,8 @@ func Test_getMachineReadableFormatForList(t *testing.T) {
 					},
 					Spec: StorageSpec{
 						Size: "100Mi",
+					},
+					Status: StorageStatus{
 						Path: "data",
 					},
 				},
@@ -179,6 +221,8 @@ func Test_getMachineReadableFormatForList(t *testing.T) {
 					},
 					Spec: StorageSpec{
 						Size: "500Mi",
+					},
+					Status: StorageStatus{
 						Path: "backend",
 					},
 				},
@@ -200,6 +244,8 @@ func Test_getMachineReadableFormatForList(t *testing.T) {
 						},
 						Spec: StorageSpec{
 							Size: "100Mi",
+						},
+						Status: StorageStatus{
 							Path: "data",
 						},
 					},
@@ -213,6 +259,8 @@ func Test_getMachineReadableFormatForList(t *testing.T) {
 						},
 						Spec: StorageSpec{
 							Size: "500Mi",
+						},
+						Status: StorageStatus{
 							Path: "backend",
 						},
 					},

pkg/storage/types.go

@@ -15,8 +15,6 @@ type Storage struct {
 // StorageSpec indicates size and path of storage
 type StorageSpec struct {
 	Size string `json:"size,omitempty"`
-	// if path is empty, it indicates that the storage is not mounted in any component
-	Path string `json:"path,omitempty"`
 }
 
 // StorageList is a list of storages
@@ -28,5 +26,6 @@ type StorageList struct {
 
 // StorageStatus is status of storage
 type StorageStatus struct {
-	Mounted bool `json:"mounted"`
+	// if path is empty, it indicates that the storage is not mounted in any component
+	Path string `json:"path,omitempty"`
 }

pkg/util/util.go

@@ -533,7 +533,7 @@ func IsGlobExpMatch(strToMatch string, globExps []string) (bool, error) {
 	return false, nil
 }
 
-// CheckOutputflag return true if specified output format is supported
+// CheckOutputFlag returns true if specified output format is supported
 func CheckOutputFlag(outputFlag string) bool {
 	if outputFlag == "json" || outputFlag == "" {
 		return true

tests/e2e/json_test.go

@@ -25,11 +25,9 @@ var _ = Describe("odojsonoutput", func() {
 
 		})
 		// Basic creation
-		It("Pre-Test Creation: Creating Application", func() {
+		It("Pre-Test Creation Json", func() {
 			runCmdShouldPass("odo app create myapp")
 			runCmdShouldPass("odo create nodejs nodejs --git https://github.com/openshift/nodejs-ex")
-			runCmdShouldPass("odo storage create mystorage --path=/opt/app-root/src/storage/ --size=1Gi")
-
 		})
 		// odo url create -o json
 		It("should be able to create url", func() {
@@ -38,7 +36,14 @@ var _ = Describe("odojsonoutput", func() {
 			desired := fmt.Sprintf(`{"kind":"url","apiVersion":"odo.openshift.io/v1alpha1","metadata":{"name":"myurl","creationTimestamp":null},"spec":{"host":"%s","protocol":"http","port":8080}}`, url)
 			areEqual, _ := compareJSON(desired, actual)
 			Expect(areEqual).To(BeTrue())
+		})
 
+		// odo storage create -o json
+		It("should be able to create storage", func() {
+			actual := runCmdShouldPass("odo storage create mystorage --path=/opt/app-root/src/storage/ --size=1Gi -o json")
+			desired := `{"kind":"storage","apiVersion":"odo.openshift.io/v1alpha1","metadata":{"name":"mystorage","creationTimestamp":null},"spec":{"size":"1Gi"},"status":{"path":"/opt/app-root/src/storage/"}}`
+			areEqual, _ := compareJSON(desired, actual)
+			Expect(areEqual).To(BeTrue())
 		})
 		// odo app describe myapp -o json
 		It("should be able to describe app", func() {
@@ -79,10 +84,11 @@ var _ = Describe("odojsonoutput", func() {
 			Expect(areEqual).To(BeTrue())
 
 		})
+
 		// odo storage list -o json
 		It("should be able to list storage", func() {
 			actual := runCmdShouldPass("odo storage list -o json")
-			desired := `{"kind":"List","apiVersion":"odo.openshift.io/v1aplha1","metadata":{},"items":[{"kind":"Storage","apiVersion":"odo.openshift.io/v1alpha1","metadata":{"name":"mystorage","creationTimestamp":null},"spec":{"size":"1Gi","path":"/opt/app-root/src/storage/"},"status":{"mounted":true}}]}`
+			desired := `{"kind":"List","apiVersion":"odo.openshift.io/v1aplha1","metadata":{},"items":[{"kind":"Storage","apiVersion":"odo.openshift.io/v1alpha1","metadata":{"name":"mystorage","creationTimestamp":null},"spec":{"size":"1Gi"},"status":{"path":"/opt/app-root/src/storage/"}}]}`
 			areEqual, _ := compareJSON(desired, actual)
 			Expect(areEqual).To(BeTrue())