Skip to content

Flavor Extra Spec Update#712

Merged
jtopjian merged 1 commit intogophercloud:masterfrom
dklyle:update-flavor-extra-specs
Jan 12, 2018
Merged

Flavor Extra Spec Update#712
jtopjian merged 1 commit intogophercloud:masterfrom
dklyle:update-flavor-extra-specs

Conversation

@dklyle
Copy link
Copy Markdown
Contributor

@dklyle dklyle commented Jan 10, 2018

For #540

Links to the line numbers/files in the OpenStack source code that support the
code in this PR:

API reference:
https://developer.openstack.org/api-ref/compute/#update-an-extra-spec-for-a-flavor

Nova implementation:
https://github.com/openstack/nova/blob/stable/pike/nova/api/openstack/compute/flavors_extraspecs.py#L79

Schema:
https://github.com/openstack/nova/blob/stable/pike/nova/api/openstack/compute/schemas/flavors_extraspecs.py#L24
https://github.com/openstack/nova/blob/stable/pike/nova/api/validation/parameter_types.py#L363-L371

The API will return an error if more than one key-value pair is passed and also if the key differs from the key parameter on the URL. As the API accepts more than one key-value pair and then properly returns an error, per the discussion on #689, the API is left to do the error handling.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jan 10, 2018

@coveralls
Copy link
Copy Markdown

coveralls commented Jan 10, 2018

Coverage Status

Coverage increased (+0.01%) to 72.912% when pulling d952b0b on dklyle:update-flavor-extra-specs into c364b06 on gophercloud:master.

@jtopjian
Copy link
Copy Markdown
Contributor

jtopjian commented Jan 11, 2018

Looking back at #689, one thing I didn't catch was that the spec key needs to be in the URL. I also tested my theory and Nova is only allowing one spec to pass. Because of these two things, I wonder if we should use the same method that server/instance metadatum creation is doing: https://github.com/gophercloud/gophercloud/blob/master/openstack/compute/v2/servers/requests.go#L646-L684.

The idea here would be to have the function signature as: flavors.UpdateExtraSpec(client, flavor.ID, updateOpts), then internally make sure only one k/v is passed and then craft the URL accordingly.

Another option is to have a signature like flavors.UpdateExtraSpec(client, flavor.ID, key, value) and craft a map internally.

Both would remove the requirement of having to specify the key twice: once in the updateOpts and once in the function.

I'm leaning on the former since it follows the usual pattern of updates in Gophercloud.

Thoughts?

@dklyle dklyle force-pushed the update-flavor-extra-specs branch from d952b0b to 35ab3f1 Compare January 11, 2018 04:56
@coveralls
Copy link
Copy Markdown

coveralls commented Jan 11, 2018

Coverage Status

Coverage decreased (-0.01%) to 72.892% when pulling 35ab3f1 on dklyle:update-flavor-extra-specs into 993bca9 on gophercloud:master.

@dklyle
Copy link
Copy Markdown
Contributor Author

dklyle commented Jan 11, 2018

I think I misunderstood your original comment about where the error should be handled. I have now updated the patch to limit accepted options to 1 key-pair on the gophercloud side and obtain the key for the URL parameter from that key. This makes more sense. It also allows update to add additional (new) extra specs, better exposing the supported nova API behavior.

@dklyle
Copy link
Copy Markdown
Contributor Author

dklyle commented Jan 11, 2018

This method rather than the second option you presented also allows potential extensibility where hard-coding both parameters would not.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jan 11, 2018

@liusheng
Copy link
Copy Markdown
Contributor

recheck

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jan 11, 2018

@jtopjian
Copy link
Copy Markdown
Contributor

I think I misunderstood your original comment about where the error should be handled.

You didn't at all - my original comment was incorrect, so it's on me - sorry about that!

This looks good. Thank you for working on this series :)

@jtopjian jtopjian merged commit af4bd1d into gophercloud:master Jan 12, 2018
@jtopjian jtopjian mentioned this pull request Jan 12, 2018
5 tasks
@liusheng
Copy link
Copy Markdown
Contributor

recheck

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci bot commented Jan 12, 2018

cardoe pushed a commit to cardoe/gophercloud that referenced this pull request Aug 27, 2020
* Internal: Networking V2 Router cleanup

Cleanup messages and variables definition for the
openstack_networking_router_v2 resource.

Implement helper functions in openstack/networking_router_v2.go with
tests.

* Remove old in-tree SDK from router helpers

Fix openstack/networking_router_v2.go imports and run go mod tidy, go
mod vendor.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants