Skip to content

Add UPDATE support in V3 volume types#656

Merged
jtopjian merged 2 commits intogophercloud:masterfrom
TommyLike:feature/volume_type_update
Dec 28, 2017
Merged

Add UPDATE support in V3 volume types#656
jtopjian merged 2 commits intogophercloud:masterfrom
TommyLike:feature/volume_type_update

Conversation

@TommyLike
Copy link
Copy Markdown
Contributor

@TommyLike TommyLike commented Dec 6, 2017 •
edited
Loading

Add Update support for volume type in volume V3.

For #649

Volume type update in V3

Volume type update Code

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 6, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.04%) to 72.798% when pulling 84acc44 on TommyLike:feature/volume_type_update into 955c2f5 on gophercloud:master.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Dec 6, 2017

Build succeeded.

@TommyLike TommyLike force-pushed the feature/volume_type_update branch from 84acc44 to e2bd2e9 Compare December 20, 2017 03:55
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Dec 20, 2017

Build succeeded.

@TommyLike TommyLike force-pushed the feature/volume_type_update branch from e2bd2e9 to 862dab9 Compare December 20, 2017 04:47
@coveralls
Copy link
Copy Markdown

coveralls commented Dec 20, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 72.819% when pulling 862dab9 on TommyLike:feature/volume_type_update into 44f115b on gophercloud:master.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Dec 20, 2017

Build succeeded.

@TommyLike
Copy link
Copy Markdown
Contributor Author

TommyLike commented Dec 20, 2017

Need to merge delete branch #655 first.

@TommyLike
Add basic CRUD support in volume v3 types
a27b5be 
Add Create/Delete/List/Update/Get support for volume
type in volume V3.
@TommyLike TommyLike force-pushed the feature/volume_type_update branch from 862dab9 to 92fb710 Compare December 21, 2017 06:32
@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Dec 21, 2017

Build succeeded.

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Dec 21, 2017

@TommyLike Looks like there's a gofmt error that Travis is flagging:

https://travis-ci.org/gophercloud/gophercloud/jobs/319561696#L885-L886

I'm not sure what editor you use, but it might be helpful to add some type of Go plugin that will automatically call gofmt upon saving. The vim-go plugin does this for vi/vim.

Otherwise, manually running gofmt on volumetypes_test.go will resolve this.

@TommyLike TommyLike force-pushed the feature/volume_type_update branch from 92fb710 to 3f3a24e Compare December 22, 2017 00:49
@TommyLike
Copy link
Copy Markdown
Contributor Author

TommyLike commented Dec 22, 2017

@jtopjian Thanks I use GoLand as my personal IDE, looks like I need to find a way to execute the go fmt command automatically before pushing the changes:)

@coveralls
Copy link
Copy Markdown

coveralls commented Dec 22, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 72.921% when pulling 3f3a24e on TommyLike:feature/volume_type_update into c2cafb4 on gophercloud:master.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Dec 22, 2017

Build succeeded.

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Dec 22, 2017

@TommyLike Can you link to the Cinder code for this?

@TommyLike
Copy link
Copy Markdown
Contributor Author

TommyLike commented Dec 22, 2017

@jtopjian oops, done@

jtopjian
jtopjian requested changes Dec 22, 2017
View reviewed changes
Copy link
Copy Markdown
Contributor

@jtopjian jtopjian left a comment

Choose a reason for hiding this comment

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

@TommyLike Overall, this looks good. Just one requested change.

openstack/blockstorage/v3/volumetypes/requests.go Outdated
type UpdateOpts struct {
Name string `json:"name,omitempty"`
Description string `json:"description,omitempty"`
IsPublic bool `json:"is_public"`
Copy link
Copy Markdown
Contributor

@jtopjian jtopjian Dec 22, 2017

Choose a reason for hiding this comment

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

This should be *bool and the json tag should be json:"is_public,omitempty"

bools are tricky to work with. Let's say someone wants to update the name of a public volume type, so they do:

updateOpts := volumetypes.UpdateOpts{
  Name: "new name",
}

But the request that would be sent to Cinder would be:

{
  "volume_type": {
    "name": "new name",
    "is_public": false
  }
}

You could use omitempty, but that means no one could ever change a volume type from public to private because false would always be omitted.

Therefore, using *bool allows for three values: true, false, and nil. omitempty prevents an empty/null field from being sent to Cinder when the value is nil.

Search the Gophercloud repo for iTrue to see how passing values to bool-pointers is usually handled.

@TommyLike
Add Update support in V3 volume type
b7a71c0 
Add Update support in V3 volume type
@TommyLike TommyLike force-pushed the feature/volume_type_update branch from 3f3a24e to b7a71c0 Compare December 25, 2017 01:24
@coveralls
Copy link
Copy Markdown

coveralls commented Dec 25, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 72.932% when pulling b7a71c0 on TommyLike:feature/volume_type_update into 05116c7 on gophercloud:master.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Dec 25, 2017

Build succeeded.

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Dec 28, 2017

@TommyLike This looks good - thank you. I'm going to follow-up with a PR to make a change to the Create options: renaming PublicAccess to IsPublic. The functionality is still the same, but it better conforms to the service-side code.

Thank you for your work on this!

@jtopjian jtopjian merged commit 614da04 into gophercloud:master Dec 28, 2017
@TommyLike TommyLike mentioned this pull request Oct 9, 2018
Open
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jtopjian jtopjian jtopjian approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@TommyLike @coveralls @jtopjian