Fix openstack NewClient to not remove relative paths from IdentityBase

[suonto]

gophercloud.provider_client.go struct ProviderClient describes IdentityBase as follows: "IdentityBase is the base URL used for a particular provider's identity service - it will be used when issuing authenticatation requests. It should point to the root resource of the identity service, not a specific identity version."

Currently gophercloud.openstack.client.go func NewClient strips endpoints with a non-empty relative path. For example, the IdentityBase returned for endpoint:

http://example.com/foo

is:

http://example.com/

when it should be:

http://example.com/foo/

This change fixes NewClient to correctly handle such cases.

Unit tests are provided in openstack.test_client.go

[1] gophercloud.openstack.client.go

Fixes-Issue: #577

[coveralls]

Coverage increased (+0.02%) to 80.567% when pulling 4c17040cf73afd1b9879528e01e9d206675b1ca4 on suonto:fix-identity-base into adc206589ed49d18cecc9890ab93534704b04702 on rackspace:master.

[jrperritt]

I think the current code is working as intended. If you know your identity endpoint, you can explicitly create a ProviderClient and set the IdentityBase/IdentityEndpoint fields

[suonto]

@jrperritt Could you please share your reasoning supporting the claim that NewClient should strip the relative paths from IdentityBase?

The justification for not stripping it is obvious: IdentityEndpoint is always constructed from IdentityBase + version. Thus, IdentityBase for endpoint http://example.com/foo/v3 should resolve to http://example.com/foo/, not http://example.com/.

Also, there was a similar relative path issue in python-openstacksdk, which was recognized and recently fixed: https://github.com/openstack/python-openstacksdk/commit/1430a8f2479cc0cd1caf72cfaecfcd84925f1227

[jrperritt]

Could you please share your reasoning supporting the claim that NewClient should strip the relative paths from IdentityBase?

I didn't claim that; I said it's working as intended. What's there works for default OpenStack implementations. Custom deployments can currently create a ProviderClient explicitly.

[suonto]

I see. I also understand that you can create client and then modify the IdentityBase, but that applies only when you are using gophercloud directly. If you are, for example, running kubernetes with openstack cloud provider, it doesn't really work that way. Anyway, the new code works exactly the same as old for default endpoints (verified by unit tests), but also supports non-default endpoints. So, could you please merge it?

[jrperritt]

I'll take a look on Monday

[coveralls]

Coverage increased (+0.4%) to 80.97% when pulling 04ac06f5eb1f296bf65cc7b125f9b69d6b98f247 on suonto:fix-identity-base into adc206589ed49d18cecc9890ab93534704b04702 on rackspace:master.

[suonto]

I preserved the regexp for a few reasons.

1. It's made for that
2. Alternative code that works as well looks quite ugly
3. I think that it's fundamentally better to find what we want and then process it, than to blindly take some part (for example the last piece of path) and then start checking if it is what we wanted
4. This is also extendable if there is ever a need for suffixes (path after matched version)

Added also more tests to verify thoroughly

[jrperritt]

Regular expressions are lazy and error-prone. They are to be used when you don't know what you'll be parsing, nor where it will be. In this case, we know both the options and the location. Because of that, using a regular expression here is not appropriate. I'd gladly review and merge this PR if you want to implement suggestion 1 in my comment above. Otherwise, it looks like we're at an impasse.

[coveralls]

Coverage increased (+0.4%) to 80.977% when pulling e481b2e70344c7a15ab2d194cd3bc6a87424b391 on suonto:fix-identity-base into adc206589ed49d18cecc9890ab93534704b04702 on rackspace:master.

[suonto]

@jrperritt have you had time to see the regexp-less implementation?

[jrperritt]

This looks good. Thank you for all the tests. +2