fix microversion header by jrperritt · Pull Request #355 · gophercloud/gophercloud

jrperritt · 2017-06-01T23:37:58Z

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

https://github.com/openstack/manila/blob/stable/newton/manila/common/config.py#L196
https://github.com/openstack/nova/blob/acfbec7db9a3df5a3183a2730510fa1f0fea92bf/nova/api/openstack/wsgi.py#L68

I think those are the only 2 services that Gophercloud currently supports that offer microversions. It does look like the X-OpenStack-<SERVICE>-API-Version headers are being replaced by OpenStack-API-Version, but are still around for backwards-compatibility, so I'm adding both. Hopefully that won't cause a problem.

This is an alternative implementation to the ones in #341 and #352 . I think it's cleaner and doesn't force users to identify the service type.

It also refactors some of the common code when making requests and creating service clients.

…service_client.go

coveralls · 2017-06-01T23:41:06Z

Coverage increased (+0.4%) to 70.418% when pulling a0c4cfe on jrperritt:53 into bbe95de on gophercloud:master.

jtopjian · 2017-06-02T02:09:53Z

I think those are the only 2 services that Gophercloud currently supports that offer microversions.

I did a quick inventory here: https://gist.github.com/jtopjian/183bf8c7cb81d0c29717a42b78b9b762

It looks like Cinder v3 supports microversions, but Gophercloud doesn't have v3 support.

Glance is questionable. No idea what it's doing.

Hopefully that won't cause a problem.

I think it's OK.

I modified the TestKeypairsList acc test with:

client.Microversion = "2.42"

and also did a dump of the request. I saw both headers and the request was successful. I then did an invalid request of:

client.Microversion = "2.52"

And successfully got:

        keypairs_test.go:26: Unable to retrieve keypairs: Expected HTTP response code [200 204] when accessing [GET http://127.0.0.1:8774/v2.1/a99e9b4e620e4db09a2dfb6e42a01e66/os-keypairs], but got 406 instead
                {"computeFault": {"message": "Version 2.52 is not supported by the API. Minimum is 2.1 and maximum is 2.42.", "code": 406}}

This implementation looks good to me. It's a very clean implementation and I was able to digest it quickly. Absolutely no offence is meant to the other PRs, though.

I think this is a fine implementation. I have some other thoughts / questions that I'll add to #53 since they're not code related, but this could easily be a foundation for some other things, if applicable.

jtopjian · 2017-06-02T02:33:10Z

Also, I'll do another review and some more testing later and then sign off on it.

jtopjian

This looks good to me. Existing acceptance tests pass and I was able to alter the microversion header and see that it was being recognized by the API server.

pospispa · 2017-06-05T10:26:56Z

service_client.go

-	opts.MoreHeaders["X-OpenStack-Nova-API-Version"] = client.Microversion
-
-	return client.Request("DELETE", url, opts)
+	opts.MoreHeaders["OpenStack-API-Version"] = client.Type + " " + client.Microversion


I would rather check whether client.Type != ""

That's reasonable

pospispa · 2017-06-05T10:43:47Z

service_client.go

+	switch client.Type {
+	case "compute":
+		opts.MoreHeaders["X-OpenStack-Nova-API-Version"] = client.Microversion
+	case "sharev2":


A nit: compute and sharev2 strings can be defined as a const as they are used on more than one place and the semantic meaning in which they are used is the same.
These constants are used in the gophercloud and openstack packages. Because openstack package imports gophercloud package the constants will have to be defined in the gophercloud package because of import cycle even though they semantically belong to openstack package.

True. That may be something to add later when there are several more client types supporting microversions.

pospispa · 2017-06-05T11:10:04Z

openstack/client.go

-	return &gophercloud.ServiceClient{ProviderClient: client, Endpoint: url}, nil
+	sc.ProviderClient = client
+	sc.Endpoint = url
+	sc.Type = clientType


It is assumed that client.EndpointLocator will return an endpoint URL that's service type equals the requested clientType. I think this is fine and it's how client.EndpointLocator works now.
However, there are other service information that are available in func V2EndpointURL and func V3EndpointURL and are not returned by the functions and that may be useful in the gophercloud.ServiceClient struct in the future.
IMHO, we may open the doors for returning these information from the functions by adding one more return value to the EndpointLocator type.

For me, what you've suggested in #352 is too complex for what's required here. I don't want to throw all information returned from an endpoint into a struct.

Currently, there is no mechanism for returning information from endpoint that would be easily extensible. That's why I'm proposing to add a struct as a new endpoint return value. The struct can be easily extended in the future and doesn't have to contain all currently available information.

As you introduced the func initClientOpts, the client.EndpointLocator is called at less places than before. So it's now easier to add new return values to the client.EndpointLocator function.

I agree with you solution.

pospispa · 2017-06-05T11:15:23Z

service_client.go

-func (client *ServiceClient) Get(url string, JSONResponse interface{}, opts *RequestOpts) (*http.Response, error) {
-	if opts == nil {
-		opts = &RequestOpts{}
+func (client *ServiceClient) initReqOpts(url string, JSONBody interface{}, JSONResponse interface{}, opts *RequestOpts) {


I like how the duplicate code is replaced by this function.

coveralls · 2017-06-05T13:21:53Z

Coverage increased (+0.5%) to 70.481% when pulling fa84c68 on jrperritt:53 into bbe95de on gophercloud:master.

jrperritt · 2017-06-05T13:58:37Z

We're going to go forward with this one. It adds the least amount of complexity while also allowing users to simply supply a service's microversion. If more information is needed by the ServiceClient in the future, we can address that then. Thank y'all for the reviews @jtopjian @pospispa.

* fix microversion header; refactor common operations in client.go and service_client.go * don't add microversion header for nil client type

fix microversion header; refactor common operations in client.go and …

a0c4cfe

…service_client.go

jrperritt requested a review from jtopjian June 1, 2017 23:37

jrperritt added the in progress label Jun 1, 2017

jrperritt mentioned this pull request Jun 1, 2017

Added Type into ServiceClient to Distinguish Different Services #341

Closed

jtopjian mentioned this pull request Jun 2, 2017

fix microversion header #53

Closed

jtopjian approved these changes Jun 3, 2017

View reviewed changes

pospispa reviewed Jun 5, 2017

View reviewed changes

don't add microversion header for nil client type

fa84c68

jrperritt merged commit 5acaa3c into gophercloud:master Jun 5, 2017

jrperritt removed the in progress label Jun 5, 2017

This was referenced Jun 5, 2017

[WIP] Fix Microversion Header #352

Closed

Get manila api microversion #342

Closed

jrperritt deleted the 53 branch June 5, 2017 16:08

j-griffith pushed a commit to j-griffith/gophercloud that referenced this pull request Jul 15, 2017

fix microversion header (gophercloud#355)

fd69fff

* fix microversion header; refactor common operations in client.go and service_client.go * don't add microversion header for nil client type

Conversation

jrperritt commented Jun 1, 2017

Uh oh!

coveralls commented Jun 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtopjian commented Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jtopjian commented Jun 2, 2017

Uh oh!

jtopjian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jun 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrperritt commented Jun 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Jun 1, 2017 •

edited

Loading

jtopjian commented Jun 2, 2017 •

edited

Loading

coveralls commented Jun 5, 2017 •

edited

Loading