Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

controller: Accept a versioned API URL #65

Merged
merged 2 commits into from
Apr 3, 2017

Conversation

babbageclunk
Copy link
Contributor

At the moment if someone tries to specify a particular API version in the URL (e.g. http://maas.server/MAAS/api/2.0/), we try to tack the version on the end again and fail to connect. Handle this by detecting the version in the URL and skipping the normal loop over supported versions.

Fix for this Juju bug: https://bugs.launchpad.net/juju/+bug/1675485

This enables creating a controller without doing version probing.
Copy link
Contributor

@howbazaar howbazaar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General approval. Just one additional test requested. Also thinking on how we can avoid treating transient network errors the same as unsupported versions.

client_test.go Outdated
expectedURL, err := url.Parse("http://example.com/MAAS/api/1.0/")
c.Assert(err, jc.ErrorIsNil)
func (suite *ClientSuite) TestAddAPIVersionToURL(c *gc.C) {
apiurl := AddAPIVersionToURL("http://example.com/MAAS", "1.0")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd also add a test for the base url having a trailing slash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wilco.

case err == nil:
return controller, nil
case IsBadVersionInfoError(err):
// TODO(babbageclunk): this is bad - it treats transient
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we treat transient network errors differently?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure - I was leaving that there for myself when I work on the next bug.

If a versioned url is used, the normal version negotiation will be
skipped.
@babbageclunk babbageclunk force-pushed the allow-versioned-url branch from f57a1dd to c8e3e67 Compare April 3, 2017 23:37
@babbageclunk
Copy link
Contributor Author

$$merge$$

@jujubot
Copy link
Contributor

jujubot commented Apr 3, 2017

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju-gomaasapi

@jujubot jujubot merged commit 0683923 into juju:master Apr 3, 2017
@babbageclunk babbageclunk deleted the allow-versioned-url branch April 3, 2017 23:40
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.

3 participants