Skip to content
This repository has been archived by the owner on Mar 26, 2020. It is now read-only.

Auto generate Volume Name if not specified in Request #876

Merged
merged 3 commits into from
Jun 25, 2018
Merged

Auto generate Volume Name if not specified in Request #876

merged 3 commits into from
Jun 25, 2018

Conversation

aravindavk
Copy link
Member

Updates: #851
Signed-off-by: Aravinda VK avishwan@redhat.com

Madhu-1
Madhu-1 reviewed Jun 11, 2018
View reviewed changes
Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, only concern is in case of volume name is empty we are logging it in CLI.

@Madhu-1
Copy link
Member

Madhu-1 commented Jun 11, 2018

@aravindavk is it possible to add a test case for this.

Madhu-1
Madhu-1 reviewed Jun 11, 2018
View reviewed changes
glustercli/cmd/volume-create.go Outdated
@@ -45,12 +46,13 @@ var (
Use: "create <volname> [<brick> [<brick>]...|--size <size>]",
Copy link
Member

@Madhu-1 Madhu-1 Jun 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to change this Use

create --name=volname  [<brick> [<brick>]...|--size <size>]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated. Thanks

@Madhu-1
Copy link
Member

Madhu-1 commented Jun 11, 2018

@aravindavk not related to this. Just a question
we have Volume struct and VolCreateReq struct in req_resp.go and VolCreateReq struct in volume_req.go. are we using all these structures for volume create?

@aravindavk
Copy link
Member Author

LGTM, only concern is in case of volume name is empty we are logging it in CLI.

That should be fine I guess, it will reflect the user input.

@aravindavk
Copy link
Member Author

@aravindavk not related to this. Just a question
we have Volume struct and VolCreateReq struct in req_resp.go and VolCreateReq struct in volume_req.go. are we using all these structures for volume create?

We are using all of the struct, but we need to unify these structs. Once we decide on API endpoint for smartvol and regular volume create, we can start unifying them

raghavendra-talur
raghavendra-talur suggested changes Jun 14, 2018
View reviewed changes
Copy link
Member

@raghavendra-talur raghavendra-talur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one case is not handled where name is provided but bricks are omitted. Rest of the changes look good.

glustercli/cmd/volume-create.go
@@ -133,17 +133,16 @@ func volumeCreateCmdRun(cmd *cobra.Command, args []string) {
return
}

if len(args) < 2 {
if len(args) < 1 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens when name is specified but bricks are omitted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converted name as flag(--name=<name>). args will contain bricks list only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@aravindavk
Copy link
Member Author

I realized that, I need to update documentation and args upgrade script. Do not merge.

I will also add test case as @Madhu-1 suggested

@aravindavk aravindavk mentioned this pull request Jun 18, 2018
Merged
aravindavk and others added 2 commits June 19, 2018 10:37
@aravindavk
Copy link
Member Author

Added tests and updated documentation. Please review

Madhu-1
Madhu-1 approved these changes Jun 25, 2018
View reviewed changes
Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aravindavk aravindavk merged commit 31f8290 into gluster:master Jun 25, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@raghavendra-talur raghavendra-talur raghavendra-talur requested changes

@prashanthpai prashanthpai prashanthpai approved these changes

@Madhu-1 Madhu-1 Madhu-1 approved these changes

@kshlm kshlm Awaiting requested review from kshlm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aravindavk @Madhu-1 @prashanthpai @raghavendra-talur