Add List/Get support for volume type in volume V3#657
Add List/Get support for volume type in volume V3#657jtopjian merged 2 commits intogophercloud:masterfrom
Conversation
Add Create/Delete/List/Update/Get support for volume type in volume V3.
|
Build succeeded.
|
jtopjian
left a comment
There was a problem hiding this comment.
@TommyLike Thank you for submitting the separate PRs.
I haven't done a full review/test on this but I found two areas that should be addressed first.
Also, with the separate PRs, it usually works best if done in the following two ways:
- Submit 1 PR and wait until it's merged before submitting the next.
- Create a branch for list/get, submit the PR, create a new branch for Create based off of list/get, submit the PR.
You're more than welcome to keep the method you have here, but it looks like you might have to do significant editing when one of the PRs is merged.
| @@ -0,0 +1,4 @@ | |||
| // Package volumetypes provides information and interaction with volume types in the | |||
| // OpenStack Block Storage service. A volume type is a collection of specs used to | |||
| // define the volume capabilities. | |||
There was a problem hiding this comment.
Can you structure this similar to the other doc.go files and include an example of how to use this package? Each PR will amend the example.
|
|
||
| *r = VolumeType(s.tmp) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This method doesn't seem to be required here. A custom UnmarshalJSON function is only to smooth/convert types. For example, converting a time field to time.Time or if a result is returned both as a int and a string.
e7f5800 to
22aad20
Compare
|
@jtopjian done! 😄 |
|
Build succeeded.
|
jtopjian
left a comment
There was a problem hiding this comment.
@TommyLike Thank you.
In addition to the developer API documentation, we also need to see the Cinder code which implements the feature. This is detailed both in the style guide and in the PR template. In this case, what we're looking for is:
https://github.com/openstack/cinder/blob/master/cinder/volume/volume_types.py
and
https://github.com/openstack/cinder/blob/master/cinder/api/v2/views/types.py#L23-L45
I mentioned before that I recommend using #583 as a template for how to structure feature additions. Notice how it is linking to the relevant Keystone code for the features being implemented.
Please make sure to include links to the service-side code in future PRs. This is a requirement.
I've left some inline comments for review. Please let me know if you have any questions.
|
|
||
| Example to show a single Volume Type | ||
|
|
||
| typeID := "0fe36e73809d46aeae6705c39077b1b3" |
There was a problem hiding this comment.
This line needs to be indented a little bit more to align with the rest of the code.
There was a problem hiding this comment.
Strange, it looks different as on my IDE.
There was a problem hiding this comment.
It might be a tab/space thing. It's common when writing code inside comment blocks. :)
| } | ||
|
|
||
| // List returns Volume types. | ||
| func List(client *gophercloud.ServiceClient) pagination.Pager { |
There was a problem hiding this comment.
It looks like this is missing ListOpts. Both the Cinder API reference documentation and Cinder code show that the List action can accept several options:
https://github.com/openstack/cinder/blob/master/cinder/volume/volume_types.py#L107-L111
If you search Gophercloud for ListOpts, you can find several examples of how to implement this.
| ExtraSpecs map[string]string `json:"extra_specs"` | ||
| // Whether the volume type is publicly visible. | ||
| IsPublic bool `json:"is_public"` | ||
| } |
There was a problem hiding this comment.
What about qos_specs_id?
You have that field in the unit tests but you aren't defining it here.
22aad20 to
f178bdb
Compare
|
Build succeeded.
|
f178bdb to
6498ec8
Compare
|
Build succeeded.
|
|
@jtopjian done 😄 |
jtopjian
left a comment
There was a problem hiding this comment.
@TommyLike This is really close! 😄
I noticed that the ListOpts has a limit option. limit usually means "multiple pages", which won't work with a SinglePageBase. SinglePageBase is used for requests that will always return all results.
I quickly verified this by doing:
#!/bin/bash
oscurl() {
token=$(openstack token issue -c id -f value)
curl -s -H "X-Auth-Token:$token" $1
}
oscurl http://localhost:8776/v3/47b5e614bb7b4842a685b98cb7048efb/types?limit=1 | python -mjson.toolAnd got the following output:
{
"volume_type_links": [
{
"href": "http://localhost:8776/v3/47b5e614bb7b4842a685b98cb7048efb/types?limit=1&marker=d2ba55cf-dd7e-4ef8-8f50-ab8c98565587",
"rel": "next"
}
],
"volume_types": [
{
"description": null,
"extra_specs": {},
"id": "d2ba55cf-dd7e-4ef8-8f50-ab8c98565587",
"is_public": true,
"name": "bar",
"os-volume-type-access:is_public": true,
"qos_specs_id": null
}
]
}So you can see that the List request is paginating when there's a limit present.
In order to resolve this, you'll need to change SinglePageBase to LinkedPageBase. You can see how to implement this in the openstack/compute/v2/flavors package.
Here's a quick diff I did to get this to work. You'll need to update the unit tests to account for this in your List test, but I've confirmed it works in an acceptance test:
diff --git a/openstack/blockstorage/v3/volumetypes/requests.go b/openstack/blockstorage/v3/volumetypes/requests.go
index 9537248..468fd86 100644
--- a/openstack/blockstorage/v3/volumetypes/requests.go
+++ b/openstack/blockstorage/v3/volumetypes/requests.go
@@ -51,6 +51,6 @@ func List(client *gophercloud.ServiceClient, opts ListOptsBuilder) pagination.Pa
}
return pagination.NewPager(client, url, func(r pagination.PageResult) pagination.Page {
- return VolumeTypePage{pagination.SinglePageBase(r)}
+ return VolumeTypePage{pagination.LinkedPageBase{PageResult: r}}
})
}
diff --git a/openstack/blockstorage/v3/volumetypes/results.go b/openstack/blockstorage/v3/volumetypes/results.go
index c2439e7..11a6ad7 100644
--- a/openstack/blockstorage/v3/volumetypes/results.go
+++ b/openstack/blockstorage/v3/volumetypes/results.go
@@ -25,7 +25,7 @@ type VolumeType struct {
// VolumeTypePage is a pagination.pager that is returned from a call to the List function.
type VolumeTypePage struct {
- pagination.SinglePageBase
+ pagination.LinkedPageBase
}
// IsEmpty returns true if a ListResult contains no Volume Types.
@@ -34,6 +34,17 @@ func (r VolumeTypePage) IsEmpty() (bool, error) {
return len(volumes) == 0, err
}
+func (page VolumeTypePage) NextPageURL() (string, error) {
+ var s struct {
+ Links []gophercloud.Link `json:"volume_type_links"`
+ }
+ err := page.ExtractInto(&s)
+ if err != nil {
+ return "", err
+ }
+ return gophercloud.ExtractNextURL(s.Links)
+}
+
// ExtractVolumeTypes extracts and returns Volumes. It is used while iterating over a volumetypes.List call.
func ExtractVolumeTypes(r pagination.Page) ([]VolumeType, error) {
var s []VolumeTypeLet me know if you have any questions. Also, thank you for sticking with this through all requested changes.
6498ec8 to
ac6f459
Compare
|
Build failed.
|
|
Thanks for your reminder @jtopjian! If that so, I guess we need to add limit support in snapshot and volume resources? |
|
/test all |
|
recheck |
|
Build failed.
|
|
recheck |
|
Build failed.
|
|
recheck |
|
Build failed.
|
|
recheck |
$ oscurl http://localhost:8776/v3/f3488662cb21447fa50d69fcadf2a921/volumes?limit=1 | python -mjson.tool
{
"volumes": [
{
"id": "e8c0142e-ae74-4a6a-94fd-aa226e5a88c3",
"links": [
{
"href": "http://localhost:8776/v3/f3488662cb21447fa50d69fcadf2a921/volumes/e8c0142e-ae74-4a6a-94fd-aa226e5a88c3",
"rel": "self"
},
{
"href": "http://localhost:8776/f3488662cb21447fa50d69fcadf2a921/volumes/e8c0142e-ae74-4a6a-94fd-aa226e5a88c3",
"rel": "bookmark"
}
],
"name": "foo2"
}
],
"volumes_links": [
{
"href": "http://localhost:8776/v3/f3488662cb21447fa50d69fcadf2a921/volumes?limit=1&marker=e8c0142e-ae74-4a6a-94fd-aa226e5a88c3",
"rel": "next"
}
]
}Yep - looks like that was missed with the |
|
@TommyLike The paging looks correct. Thanks! The only thing that needs updated is the unit test for list. The unit test is passing because you're mocking an incorrect request and response. See the |
|
Build succeeded.
|
Add list/show support in V3 volume type
ac6f459 to
205e3c7
Compare
|
Build succeeded.
|
|
Looks good. Thank you for your hard work with this. As well, thank you for spotting the missing |
Add List/Get support for volume type in volume V3.
For #649
Document:
Volume type get V3
Volume type list V3
Code:
Volume type get V3
Volume type list V3
Model:
Link