Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Remove environs/tools.FindInstanceTools #649
Conversation
|
This PR needs a description... |
mjs
reviewed
Sep 1, 2014
| + if params.Constraints.Arch != nil { | ||
| + filter.Arch = *params.Constraints.Arch | ||
| + } | ||
| + possibleTools, err := tools.FindTools(env, -1, -1, filter, false) |
mjs
Sep 1, 2014
Contributor
This is probably outside the scope of this PR but I wish the -1's and false where defined as consts (in tools or coretools) with self documenting names.
mjs
Sep 1, 2014
Contributor
I've just noticed there is a DoNotAllowRetry in environs/tools which is intended for the last parameter.
axw
Sep 2, 2014
Member
Thanks for the reminder, used DoNotAllowRetry. Leaving the -1's alone for now, but agree they could do with some description. The AllowRetry/DoNotAllowRetry constants (and the parameter) will be going away.
mjs
reviewed
Sep 1, 2014
| - tools, err := findInstanceTools(env, machine.Series(), *hc.Arch) | ||
| + agentVersion, ok := env.Config().AgentVersion() | ||
| + if !ok { | ||
| + return nil, fmt.Errorf("no agent version set in environment configuration") |
mjs
reviewed
Sep 1, 2014
| + expectedList, err := tools.FindTools(s.Environ, -1, -1, coretools.Filter{ | ||
| + Number: currentVersion.Number, | ||
| + Series: currentVersion.Series, | ||
| + }, false) |
|
LGTM with only minor comments. My review mentor @howbazaar needs to do a meta-review however. |
|
Review by @mjs looks good to me. |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Generating tarball failed |
|
$$tryagain$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
added a commit
that referenced
this pull request
Sep 2, 2014
axw commentedSep 1, 2014
Reduce surface area of environs/tools as we reduce reliance on it. Eventually the simplestreams parts should be relocated and become client-side, import-only. Several users do not need to be using FindInstanceTools (use FindExactTools or just FindTools instead).