Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Add login v2. Stop serving the environment at the root of the API. #1868
Conversation
mjs
reviewed
Mar 18, 2015
| +func OpenWithVersion(info *Info, opts DialOpts, loginVersion int) (*State, error) { | ||
| + switch loginVersion { | ||
| + case 0: | ||
| + return open(info, opts, (*State).loginV0) |
mjs
Mar 18, 2015
Contributor
Nit: given that the case blocks are all the same except for the login method arg, I'd prefer to see the login method assigned to a variable in each case block, with open called just once at the bottom of the func.
Not a biggie though.
mjs
reviewed
Mar 18, 2015
| + return open(info, opts, (*State).Login) | ||
| +} | ||
| + | ||
| +// This unexported open method is used both directly above in the Open |
mjs
reviewed
Mar 18, 2015
| + } | ||
| + path = fmt.Sprintf("/environment/%s/log", envTag.Id()) | ||
| + } | ||
| + | ||
| target := url.URL{ |
mjs
Mar 18, 2015
Contributor
Just noticed that the following bit can all be replaced with a call to api.Connect. It's not for this PR though.
Actually... it's a bit more than just Connect, but client connections to /log and /logsink should share a common function. I'll take care of that in a future PR.
mjs
reviewed
Mar 18, 2015
| + return &restrictedRoot{finder} | ||
| +} | ||
| + | ||
| +var restrictedRootNames = set.NewStrings( |
mjs
reviewed
Mar 18, 2015
| +// of the facades available at the server root when there is no active | ||
| +// environment. | ||
| +func (r *restrictedRoot) FindMethod(rootName string, version int, methodName string) (rpcreflect.MethodCaller, error) { | ||
| + if !restrictedRootNames.Contains(rootName) { |
mjs
Mar 18, 2015
Contributor
The problem with this approach is that for completely invalid root names the client gets the same error as if they tried to access a root that's no longer available.
The other roots (e.g. upgradingRoot) do FindMethod first and let that generate a specific error for completely wrong calls, and then filter. Consider doing the same thing here.
mjs
reviewed
Mar 18, 2015
| @@ -0,0 +1,92 @@ | ||
| +Logging in to the API Server |
mjs
reviewed
Mar 18, 2015
| + | ||
| +Implementation Notes: | ||
| + | ||
| +The login process for both v0 and v1 logins are handled in the method *admin.doLogin (in admin.go). The method *Conn.ServeFinder on the *rpc.Conn attribute of the apiHandler is initially given the anonymous root here: |
mjs
Mar 18, 2015
Contributor
Line lengths get longer from here on. Make them consistent with the rest of the doc?
|
My comments are all fairly minor. LGTM. |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$my-fault$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$replicaset-failure$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
added a commit
that referenced
this pull request
Mar 19, 2015
howbazaar commentedMar 18, 2015
This change set has a bunch of mostly related changes.
I added a ServerVersion to the login response. This is set for the version two login.
The creation of some machines and units in tests were simplified to use the factory.
The api package gains a new exported method: OpenWithVersion that allows specifying the version of the Login to use. This is only used in tests and allows the client to pretend to be older.
(Review request: http://reviews.vapour.ws/r/1247/)