Add odo storage list --all command feature #568

piyush-garg · 2018-07-07T21:59:02Z

This will list all the storages in all the components in a
current application and project
Syntax will be odo storage list --all
on execution of odo storage list --all componentName it will fail

Fixes #507

mik-dass · 2018-07-10T07:20:30Z

cmd/storage.go

+				fmt.Println("Invalid arguments. component name is not needed")
+				os.Exit(1)
+			}
+			componentList := getComponentList(client, applicationName, projectName)


I guess we can directly use component.List here instead of creating a new function in cmdutils

mik-dass · 2018-07-10T07:21:26Z

cmd/cmdutils.go

@@ -60,6 +62,13 @@ func getComponent(client *occlient.Client, inputComponent, applicationName, proj
 	return inputComponent
 }

+// getComponentList returns the list of all components in current application and project
+func getComponentList(client *occlient.Client, applicationName, projectName string) []component.ComponentInfo {


I guess we can directly use component.List instead of using this function

mik-dass · 2018-07-10T07:43:12Z

The output looks like

The component 'nodejs' has the following storage attached -
NAME          SIZE     PATH
backend-1     100M     /data-1

No unmounted storage exists to mount to 'nodejs' 
The component 'wildfly' has the following storage attached -
NAME        SIZE     PATH
backend     100M     /data

No unmounted storage exists to mount to 'wildfly'

The No unmounted storage exists part appears twice and I think it's unnecessary to display it more than once, so I guess we can have a different design layout for the --all case

cdrage · 2018-07-10T18:16:19Z

cmd/cmdutils.go

@@ -60,6 +62,13 @@ func getComponent(client *occlient.Client, inputComponent, applicationName, proj
 	return inputComponent
 }

+// getComponentList returns the list of all components in current application and project


in the current application and project

cdrage · 2018-07-10T18:16:45Z

cmd/cmdutils.go

@@ -60,6 +62,13 @@ func getComponent(client *occlient.Client, inputComponent, applicationName, proj
 	return inputComponent
 }

+// getComponentList returns the list of all components in current application and project
+func getComponentList(client *occlient.Client, applicationName, projectName string) []component.ComponentInfo {


should be string for applicationName: applicationName string

cdrage · 2018-07-10T18:20:05Z

cmd/cmdutils.go

+	fmt.Fprintln(tabWriterMounted, "NAME", "\t", "SIZE", "\t", "PATH")
+	fmt.Fprintln(tabWriterUnmounted, "NAME", "\t", "SIZE")
+
+	for _, storage := range storageList {


need to add comments all here descripting what's going on. explaining that this is creating a table, etc.

cdrage · 2018-07-10T18:21:17Z

cmd/cmdutils.go

+	} else {
+		fmt.Printf("The component '%v' has no storage attached\n", componentName)
+	}
+	fmt.Println("")


this is kind of odd.. maybe remove the newline being adding and add newline to the other fmt.Printf functions below?

cdrage · 2018-07-10T18:22:34Z

cmd/storage.go

-				fmt.Fprintln(tabWriterUnmounted, storage.Name, "\t", storage.Size)
+		if storageAllListflag {
+			if storageComponent != "" {
+				fmt.Println("Invalid arguments. component name is not needed")


capitalize "Component"

cdrage · 2018-07-10T18:25:51Z

tests/e2e/e2e_test.go

+				Expect(storList).To(ContainSubstring("pv2"))
+			})
+
+			// TODO: Verify if the storage removed using odo deletes pvc


does this need to removed?

@cdrage It's not the part of my PR, not aware

kadel · 2018-07-24T13:04:15Z

Storage that is not mounted should be grouped together at the end of the output.
How about something like this for output?

The component 'nodejs' has no storage attached.

The component 'wildfly' has the following storage attached:
NAME             SIZE     PATH
wildfly-alfa     1G       /data

The component 'php' has the following storage attached:
NAME             SIZE     PATH
php-asdf         1G       /www

Storage that is not mounted to any component:
NAME             SIZE
wildfly-vxgn     1G
mydata           1G

surajnarwade

Tested this PR, LGTM
only this small change

surajnarwade · 2018-07-30T09:24:19Z

cmd/cmdutils.go

+	}
+	//print all mounted storages of the given component
+	if hasMounted {
+		fmt.Printf("The component '%v' has the following storage attached -\n", componentName)


put : instead of -

piyush-garg · 2018-07-30T09:47:33Z

@surajnarwade Fixed

mik-dass

The printStoragesInAllComponent needs a fix

mik-dass · 2018-07-30T12:24:02Z

cmd/cmdutils.go

+				if !hasUnmounted {
+					hasUnmounted = true
+				}
+				fmt.Fprintln(tabWriterUnmounted, storage.Name, "\t", storage.Size)


Storage.List() returns the unmounted storage in the same application. So if there are two or more components in the same application and one or more unmounted storage in the application, then this will display the same unmounted storage more than once as the unmounted storage are getting written to the tabWriterUnmounted more than once

[mdas@localhost odo]$ ./odo storage list --all The component 'nodejs' has the following storage attached - NAME SIZE PATH backend-nodejs 100M /data-1 The component 'wildfly' has no storage attached Storages that are not mounted to any component - NAME SIZE backend 100M backend-wild 100M backend 100M backend-wild 100M

piyush-garg · 2018-07-30T13:16:32Z

@cdrage Can you please review it again?

mik-dass · 2018-07-30T17:20:16Z

cmd/storage.go

-			fmt.Printf("No unmounted storage exists to mount to '%v' \n", componentName)
+			//storageComponent is the input component name
+			componentName := getComponent(client, storageComponent, applicationName, projectName)
+			if componentName == "" {


This check is not required as getComponent() does the same thing

mik-dass

LGTM :)

piyush-garg · 2018-07-31T13:32:52Z

@ashetty1 Does E2E test change look good to you?

ashetty1 · 2018-08-01T07:04:13Z

@piyush1594 yes, tests LGTM

kadel · 2018-08-01T10:23:13Z

Wouldn't it be better and easier to modify storage.List to list just mounted storage in the given component, and create new storage.ListUnmounte function that will list all unmounted volumes in the given application?

kadel · 2018-08-09T08:57:25Z

@cdrage can you please check if your comments were addressed?

I think that we can merge it after that. (even though it could be solved a bit more elegant way 😇 )

piyush-garg · 2018-08-09T12:05:26Z

@kadel @mik-dass @cdrage Will update the PR in a day or two. Is it fine with you?

kadel · 2018-08-10T10:07:02Z

@kadel @mik-dass @cdrage Will update the PR in a day or two. Is it fine with you?

you don't really have to, If others are OK with that we can merge it, and improve it in separate PR.

piyush-garg · 2018-08-13T15:18:23Z

@cdrage Thanks a lot for the review. Changes addressed

kadel · 2018-08-16T14:27:57Z

pkg/storage/storage.go

@@ -61,7 +61,7 @@ func Create(client *occlient.Client, name string, size string, path string, comp
 }

 // Unmount unmounts the given storage from the given component
-func Unmount(client *occlient.Client, storageName string, componentName string, applicationName string) error {
+func Unmount(client *occlient.Client, storageName string, componentName string, applicationName string, updateLabels bool) error {


we need a comment that will explain what updateLabels does

code changed a lot

kadel · 2018-08-16T14:45:23Z

pkg/storage/storage.go

 	}
 	var storageList []StorageInfo
 	for _, pvc := range pvcs {
 		pvcComponentName, ok := pvc.Labels[componentlabels.ComponentLabel]
 		pvcAppName, okApp := pvc.Labels[applabels.ApplicationLabel]
-		// first check if storage label does not exists indicating that the storage is not mounted in any component
-		// if the storage label exists, then check if the component is the current active component
+		// first check if component label does not exists indicating that the storage is not mounted in any component


wasn't it correct before? It is checking labels on storage not on component

it is checking the component label on storage so changed that way @kadel

piyush-garg · 2018-08-16T14:55:28Z

@cdrage Can you please take a look again? TIA

surajnarwade

LGTM :)

mik-dass · 2018-08-20T12:32:23Z

cmd/cmdutils.go

+
+	//iterating over all the components in the given aplication and project
+	for _, component := range componentList {
+		componentName := component.Name


Can we reuse printStorageInComponent() here? Maybe by adding a flag in printStorageInComponent() called printUnmounted to print the unmounted storage part for the component

mik-dass · 2018-08-20T12:33:14Z

cmd/cmdutils.go

+	storageListMounted, err := storage.ListMounted(client, componentName, applicationName)
+	checkError(err, "Could not get mounted storage list")
+
+	//iterating over all mounted storage and put in the mount storage table


Please add a space before all the comments like "// iterating ....."

mik-dass · 2018-08-20T12:33:47Z

cmd/cmdutils.go

+	fmt.Fprintln(tabWriterUnmounted, "NAME", "\t", "SIZE")
+
+	storageListMounted, err := storage.ListMounted(client, componentName, applicationName)
+	checkError(err, "Could not get mounted storage list")


please start the error messages with a small letter

This will list all the storages in all the components in a current application and project Syntax will be odo storage list --all on execution of odo storage list --all componentName it will fail Fixes redhat-developer#507 Add e2e test related to feature Refactored the output format for --all flag Add comments about code flow Fix semicolon comment Fix issue of unmounted storage getting printed multiple times Add function for listing mounted storage and unmounted storage Add spacing before lines Add a flag in unmount function Basically in case of deleting pvc, we are first unmounting and then deleting. Then unmounting function is updating the labels So if we are deleting the pvc then updating call is not required Added a flag to update Labels or not, when delete function is called its set to false and when unmount is called it is set to true Add comments about updateLabels Made the code resuable and more simpler Address other indentation and format changes Make print format look good

mik-dass · 2018-08-23T07:55:03Z

cmd/cmdutils.go

@@ -103,3 +105,68 @@ func validateStoragePath(client *occlient.Client, storagePath, componentName, ap
 	}
 	return nil
 }
+
+// printMountedStorageInComponent prints all the mounted storage in a given component of the application
+func printMountedStorageInComponent(client *occlient.Client, applicationName string, componentName string) {


Please move componentName before applicationName

mik-dass

LGTM :)

mik-dass · 2018-08-23T13:58:40Z

@cdrage Can we merge this?

cdrage · 2018-08-23T15:07:24Z

Yeah, this LGTM. Thanks for addressing my comments!

piyush-garg · 2018-08-23T17:32:36Z

Thanks @cdrage @mik-dass @kadel @surajnarwade

piyush-garg force-pushed the iss_507 branch 3 times, most recently from 5b6f979 to 1ad4999 Compare July 8, 2018 05:42

mik-dass reviewed Jul 10, 2018

View reviewed changes

cdrage suggested changes Jul 10, 2018

View reviewed changes

piyush-garg force-pushed the iss_507 branch 5 times, most recently from b2a6a34 to edd9d4e Compare July 27, 2018 22:28

surajnarwade reviewed Jul 30, 2018

View reviewed changes

piyush-garg force-pushed the iss_507 branch from edd9d4e to 1e34e60 Compare July 30, 2018 09:35

mik-dass suggested changes Jul 30, 2018

View reviewed changes

piyush-garg force-pushed the iss_507 branch from 1e34e60 to 320822c Compare July 30, 2018 13:09

mik-dass reviewed Jul 30, 2018

View reviewed changes

piyush-garg force-pushed the iss_507 branch from 320822c to 54c3e2f Compare July 30, 2018 17:29

mik-dass previously approved these changes Jul 30, 2018

View reviewed changes

kadel added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Aug 13, 2018

piyush-garg force-pushed the iss_507 branch from 54c3e2f to e872f5b Compare August 13, 2018 13:17

piyush-garg force-pushed the iss_507 branch 2 times, most recently from d9ac235 to 2028bdb Compare August 13, 2018 15:16

piyush-garg force-pushed the iss_507 branch 4 times, most recently from be2f7c3 to e98e5ab Compare August 16, 2018 13:47

kadel reviewed Aug 16, 2018

View reviewed changes

piyush-garg force-pushed the iss_507 branch from e98e5ab to 5efef6f Compare August 16, 2018 14:44

kadel reviewed Aug 16, 2018

View reviewed changes

surajnarwade approved these changes Aug 17, 2018

View reviewed changes

kadel requested review from mik-dass and cdrage August 20, 2018 08:52

mik-dass reviewed Aug 20, 2018

View reviewed changes

piyush-garg force-pushed the iss_507 branch from 5efef6f to 75613be Compare August 20, 2018 16:54

piyush-garg force-pushed the iss_507 branch from 75613be to bc18ec7 Compare August 20, 2018 17:13

mik-dass reviewed Aug 23, 2018

View reviewed changes

Changes the order of component name and application name

8f14e3e

mik-dass approved these changes Aug 23, 2018

View reviewed changes

cdrage approved these changes Aug 23, 2018

View reviewed changes

cdrage merged commit 7965a83 into redhat-developer:master Aug 23, 2018

surajnarwade mentioned this pull request Aug 24, 2018

v0.10.0 #661

Merged

Add odo storage list --all command feature #568

Add odo storage list --all command feature #568

Conversation

piyush-garg commented Jul 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel commented Jul 24, 2018

surajnarwade left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piyush-garg commented Jul 30, 2018

mik-dass left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piyush-garg commented Jul 30, 2018

Choose a reason for hiding this comment

mik-dass left a comment

Choose a reason for hiding this comment

piyush-garg commented Jul 31, 2018

ashetty1 commented Aug 1, 2018

kadel commented Aug 1, 2018

kadel commented Aug 9, 2018

piyush-garg commented Aug 9, 2018

kadel commented Aug 10, 2018

piyush-garg commented Aug 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piyush-garg commented Aug 16, 2018

surajnarwade left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass left a comment

Choose a reason for hiding this comment

mik-dass commented Aug 23, 2018

cdrage commented Aug 23, 2018

piyush-garg commented Aug 23, 2018