Adds the functionality of unmounting storage using the mounted path. #492

mik-dass · 2018-05-31T11:25:26Z

Issue : #493
Signed-off-by: mdas mrinald7@gmail.com

surajnarwade · 2018-06-04T10:23:46Z

@mik-dass , code looks good. Please check whether you can add tests or not

kadel · 2018-06-04T10:20:52Z

cmd/storage.go

-			fmt.Printf("Storage %v does not exist in component %v\n", storageName, componentName)
-			os.Exit(1)
+		storageName := ""
+		if string(args[0][0]) == "/" {


It might be nice to have command explaining what we are doing here.

cdrage · 2018-06-04T14:41:01Z

What would happen if we pass in: ./ or something that is not valid (or does not exist)? Shouldn't we error out to the user?

mik-dass · 2018-06-05T07:02:10Z

@cdrage You mean at the start of the path?

cdrage · 2018-06-05T13:07:59Z

@mik-dass Yes

mik-dass · 2018-06-06T07:03:10Z

@cdrage I think it might be complicated as there are many combinations which are possible

kadel · 2018-06-07T08:42:29Z

What would happen if we pass in: ./ or something that is not valid (or does not exist)? Shouldn't we error out to the user?

This should be treated as a storage name, and it should fail because no such storage exits.

kadel

Use for unmount command needs to be updated.

Use:   "unmount [storage name]",

Also, Long description should explain that this command can be called with a path and that path needs to be absolute.

kadel · 2018-06-07T11:33:26Z

cmd/storage.go

-	Use:   "unmount [storage name]",
-	Short: "Unmount storage from the current component",
-	Long: `Unmount storage from the current component.
+	Use:   "unmount [storage/path name]",


we can update this based on #516
"PATH | STORAGE_NAME"

kadel · 2018-06-07T11:35:50Z

cmd/storage.go

-	Long: `Unmount storage from the current component.
+	Use:   "unmount [storage/path name]",
+	Short: "Unmount storage or storage with the given path, from the current component",
+	Long: `Unmount storage or storage with the given path, from the current component.


How about this:
Unmount storage from the given path or unmount storage identified by its name.

mik-dass · 2018-06-07T15:58:12Z

@kadel Done

cdrage · 2018-06-07T17:06:51Z

cmd/storage.go

-	Use:   "unmount [storage name]",
-	Short: "Unmount storage from the current component",
-	Long: `Unmount storage from the current component.
+	Use:   "unmount [PATH | STORAGE_NAME]",


this should be lowercase, other sources (such as create.go) use lowercase.

unmount <path | storage_name> maybe better?

@cdrage see #516

there are two options, either uppercase or lowercase in <>

@mik-dass it can't be in [] as it means that it is optional. Please check rules in #516

@kadel So between <>?

As described in docopts docs (link in #516)

either just capital letters

unmount PATH | STORAGE_NAME

or in <>

unmount <path> | <storage_name>

I would use the capital letters, it seems to be more widly used (heroku and could foundry).
We should pick one and stick with it across all commands in OpenShift-Do.

@kadel OK I will changed to the Upper case approach

cdrage · 2018-06-07T17:08:07Z

cmd/storage.go

-		if !exists {
-			fmt.Printf("Storage %v does not exist in component %v\n", storageName, componentName)
-			os.Exit(1)
+		storageName := ""


if this starts as blank, I think var storageName string is more go-esque

cdrage · 2018-06-07T17:08:48Z

cmd/storage.go

+		if string(args[0][0]) == "/" {
+			path := args[0]
+			storageName, err = storage.GetStorageNameFromMountPath(client, path, componentName, applicationName)
+			checkError(err, "")


instead of "" maybe add explanation (Unable to get storage name from mount path) or something similar?

cdrage · 2018-06-07T17:11:31Z

cmd/storage.go

+		} else {
+			storageName = args[0]
+			exists, err := storage.IsMounted(client, storageName, componentName, applicationName)
+			checkError(err, "")


same here, maybe add own explanation?

cdrage · 2018-06-07T17:11:58Z

pkg/storage/storage.go

+func GetStorageNameFromMountPath(client *occlient.Client, path string, componentName string, applicationName string) (string, error) {
+	storages, err := List(client, componentName, applicationName)
+	if err != nil {
+		return "", errors.Wrap(err, "unable to list storages")


storage not storages

@cdrage But we refer to one PVC as storage

@mik-dass Storage is an uncountable plural, we just use "storage" not "storages". https://en.wiktionary.org/wiki/storage

@cdrage I know about the plural form but here a single PVC is called storage and all of them together is also called storage. It seemed confusing. So I added the 's' to distinguish. Anyways updating it.

cdrage · 2018-06-11T12:59:59Z

LGTM

…A function GetStorageNameFromMountPath is added which returns the storageName mounted to the given path in the given component and application or returns the error. Signed-off-by: mdas <mrinald7@gmail.com>

mik-dass force-pushed the unmount_path branch from b1fb4ce to 2b6b687 Compare May 31, 2018 12:00

kadel reviewed Jun 4, 2018

View reviewed changes

mik-dass force-pushed the unmount_path branch from 2b6b687 to 18f5850 Compare June 4, 2018 10:42

kadel requested changes Jun 7, 2018

View reviewed changes

mik-dass force-pushed the unmount_path branch from 18f5850 to d5dad15 Compare June 7, 2018 09:40

kadel reviewed Jun 7, 2018

View reviewed changes

mik-dass force-pushed the unmount_path branch from d5dad15 to d9597a5 Compare June 7, 2018 15:57

cdrage suggested changes Jun 7, 2018

View reviewed changes

mik-dass force-pushed the unmount_path branch 2 times, most recently from baa2fb7 to 722b9e1 Compare June 11, 2018 09:49

cdrage approved these changes Jun 11, 2018

View reviewed changes

Adds the functionality of unmounting storage using the mounted path. …

529f5de

…A function GetStorageNameFromMountPath is added which returns the storageName mounted to the given path in the given component and application or returns the error. Signed-off-by: mdas <mrinald7@gmail.com>

mik-dass force-pushed the unmount_path branch from 722b9e1 to 529f5de Compare June 11, 2018 13:18

kadel approved these changes Jun 11, 2018

View reviewed changes

kadel merged commit f528d59 into redhat-developer:master Jun 11, 2018

mik-dass deleted the unmount_path branch June 13, 2018 10:39

snyk-bot mentioned this pull request Aug 13, 2021

[Snyk] Upgrade css-what from 5.0.0 to 5.0.1 adamkeinan/odo#2

Open

This was referenced Sep 30, 2022

[Snyk] Fix for 16 vulnerabilities adamkeinan/odo#50

Open

[Snyk] Fix for 16 vulnerabilities adamkeinan/odo#51

Open

[Snyk] Fix for 16 vulnerabilities adamkeinan/odo#52

Open

adamkeinan mentioned this pull request Oct 7, 2022

[Snyk] Fix for 16 vulnerabilities adamkeinan/odo#53

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds the functionality of unmounting storage using the mounted path. #492

Adds the functionality of unmounting storage using the mounted path. #492

mik-dass commented May 31, 2018 •

edited

surajnarwade commented Jun 4, 2018

kadel Jun 4, 2018

cdrage commented Jun 4, 2018 •

edited

mik-dass commented Jun 5, 2018

cdrage commented Jun 5, 2018

mik-dass commented Jun 6, 2018

kadel commented Jun 7, 2018

kadel left a comment •

edited

kadel Jun 7, 2018

kadel Jun 7, 2018

mik-dass commented Jun 7, 2018

cdrage Jun 7, 2018

kadel Jun 8, 2018 •

edited

kadel Jun 8, 2018

mik-dass Jun 11, 2018

kadel Jun 11, 2018

mik-dass Jun 11, 2018

cdrage Jun 7, 2018

cdrage Jun 7, 2018

cdrage Jun 7, 2018

cdrage Jun 7, 2018

mik-dass Jun 8, 2018

cdrage Jun 8, 2018 •

edited

mik-dass Jun 11, 2018 •

edited

cdrage commented Jun 11, 2018

Adds the functionality of unmounting storage using the mounted path. #492

Adds the functionality of unmounting storage using the mounted path. #492

Conversation

mik-dass commented May 31, 2018 • edited

surajnarwade commented Jun 4, 2018

Choose a reason for hiding this comment

cdrage commented Jun 4, 2018 • edited

mik-dass commented Jun 5, 2018

cdrage commented Jun 5, 2018

mik-dass commented Jun 6, 2018

kadel commented Jun 7, 2018

kadel left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass commented Jun 7, 2018

Choose a reason for hiding this comment

kadel Jun 8, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage Jun 8, 2018 • edited

Choose a reason for hiding this comment

mik-dass Jun 11, 2018 • edited

Choose a reason for hiding this comment

cdrage commented Jun 11, 2018

mik-dass commented May 31, 2018 •

edited

cdrage commented Jun 4, 2018 •

edited

kadel left a comment •

edited

kadel Jun 8, 2018 •

edited

cdrage Jun 8, 2018 •

edited

mik-dass Jun 11, 2018 •

edited