Add CREATE support in V3 volume types#654
Conversation
cfddd65 to
dba49d3
Compare
|
Build succeeded.
|
|
@TommyLike, thanks for splitting this up into the individual CRUD components. Could you also link to the source code of the API as well. API docs are not always complete or accurate. I have found from experience that there are aspects of the API implementation that can be overlooked by just relying on the public documentation. I would like to review these PRs but more complete code linking would help tremendously. Thank you. From a first run through, the code looks to be in good shape. |
|
@TommyLike I agree with @dklyle here. I just merged #657 so this PR will also need rebased to account for the changes. I suggest not rebasing the Update and Delete PRs yet. Let's work on these one at a time. Get/List is usually the biggest PR in a CRUD set, so the rest of these should be more small and easier :) |
dba49d3 to
0826245
Compare
|
Build succeeded.
|
0826245 to
ed90ba5
Compare
ed90ba5 to
5f0568f
Compare
|
Build succeeded.
|
|
@jtopjian done! |
|
@dklyle Would you like to do a review of this? |
|
@jtopjian yes, working on it now. |
There was a problem hiding this comment.
This looks very good. There is one missing option to create that should be added before merging this.
I also wonder about acceptance tests for this functionality. I looked and noticed there are no blockstorage/v3 acceptance tests yet. I'm unsure if there is a technical limitation as to why those are not there. I'm sure @jtopjian would know the answer. If not, it would be nice to start building those out as well.
| Description string `json:"description,omitempty"` | ||
| // the ID of the existing volume snapshot | ||
| PublicAccess bool `json:"os-volume-type-access:is_public"` | ||
| } |
There was a problem hiding this comment.
It appears that a supported parameter, extra_specs, is missing.
See: https://github.com/openstack/cinder/blob/master/cinder/api/contrib/types_manage.py#L61
and https://developer.openstack.org/api-ref/block-storage/v3/#create-a-volume-type
There was a problem hiding this comment.
Looking a little further, following the trail down. It looks like the extra specs get stored in a separate table as part of the process https://github.com/openstack/cinder/blob/master/cinder/db/sqlalchemy/api.py#L3474 with the model being https://github.com/openstack/cinder/blob/master/cinder/db/sqlalchemy/models.py#L478
But it just comes through this API as a dict of key:value pairs.
There's no limitation. I've been lenient with acceptance tests but I think we should start requiring them for feature additions like this. There's a bit of a learning curve to getting an environment up and running and such, but there's enough documentation now to start enforcing it. Fixing a bug or small single-field patches might be ok to let slide. I have a test for Volume Type get/list which I'll commit now so this PR can add to it. @TommyLike once #675 is merged, can you rebase with master to add a Technically all other v2 acceptance tests can be copied to v3 as well at some point. edit: If you need to set up an environment for acceptance testing, see the documentation here. |
Add Create/Delete/List/Update/Get support for volume type in volume V3.
|
@TommyLike Yep, that's totally fine. For now, the test will create the type but not delete it. When you work on the Delete PR, you can modify the acceptance test to perform the delete. :) |
Add create API in V3's volume type resource
5f0568f to
db19781
Compare
|
Build succeeded.
|
dklyle
left a comment
There was a problem hiding this comment.
Thanks @TommyLike, this looks good now.
|
All looks good. @TommyLike thank you for your work on this. @dklyle thank you for reviewing. |
…hanged (gophercloud#654) * Manila: recreate share resource, when the region is changed * Manila: recreate share access resource, when the region is changed
Add Create support for volume type in volume V3.
For #649
Document:
Volume type create V3
Code:
Volume type create V3
Model:
Link