Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd/modelcmd: remove SetAPIOpener #7254
Conversation
cmars
reviewed
Apr 19, 2017
Please do not land this unless you're sure it won't break romulus commands.
|
!!build!! |
jameinel
approved these changes
Apr 20, 2017
I can't speak to Casey's concern about Romulus, both otherwise LGTM
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
jujubot
merged commit 552e2f1
into
juju:develop
Apr 21, 2017
1 check passed
github-check-merge-juju
Built PR, ran unit tests, and tested LXD deploy. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
rogpeppe commentedApr 19, 2017
We have both SetAPIOpen and SetAPIOpener which are mutually redundant
and confusing. SetAPIOpener is barely used, so remove it and mock
api.Open instead, which means that more logic is tested.
Also move the NewTimeoutOpener logic into the kill command, the
only place that uses it, so we avoid using the APIOpener abstraction
in that case. At some point in the future, it would be nice to make
juju.NewAPIConnection cancellable (for example by passing
in a context.Context) which would make the connection timeout
logic more generally available.
Also add a modelcmd.InnerCommand function so that we can
inspect fields in wrapped commands without undue shenanigans.
QA no regressions; in particular, check that we can still use the juju kill
command to destroy a non-responsive controller.