Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in

Issue 21940044: cmd/juju: upgrade-juju uses the API (Closed)

Can't Edit
Can't Publish+Mail
Start Review
6 years, 7 months ago by dimitern
6 years, 7 months ago
jameinel, axw, mp+195626, rog


cmd/juju: upgrade-juju uses the API This adds a single needed API call: SetEnvironAgentVersion(), and uses the already existing EnvironmentGet() to implement the upgrade-juju command. https://code.launchpad.net/~dimitern/juju-core/202-cli-upgrade-juju-api/+merge/195626 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 7

Patch Set 2 : cmd/juju: upgrade-juju uses the API #

Unified diffs Side-by-side diffs Delta from patch set Stats (+73 lines, -5 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/upgradejuju.go View 1 4 chunks +15 lines, -5 lines 0 comments Download
M state/api/client.go View 2 chunks +8 lines, -0 lines 0 comments Download
M state/api/params/params.go View 2 chunks +7 lines, -0 lines 0 comments Download
M state/apiserver/client/client.go View 1 1 chunk +5 lines, -0 lines 0 comments Download
M state/apiserver/client/client_test.go View 2 chunks +12 lines, -0 lines 0 comments Download
M state/apiserver/client/perm_test.go View 3 chunks +24 lines, -0 lines 0 comments Download


Total messages: 5
Please take a look.
6 years, 7 months ago (2013-11-18 16:15:25 UTC) #1
On 2013/11/18 16:15:25, dimitern wrote: > Please take a look. LGTM
6 years, 7 months ago (2013-11-19 09:37:55 UTC) #2
LGTM I think this change is ok as is, though I'd like us to keep ...
6 years, 7 months ago (2013-11-19 11:30:58 UTC) #3
LGTM with similar reservations to John. https://codereview.appspot.com/21940044/diff/1/cmd/juju/upgradejuju.go File cmd/juju/upgradejuju.go (right): https://codereview.appspot.com/21940044/diff/1/cmd/juju/upgradejuju.go#newcode175 cmd/juju/upgradejuju.go:175: available, err := ...
6 years, 7 months ago (2013-11-19 12:37:58 UTC) #4
6 years, 7 months ago (2013-11-19 16:15:27 UTC) #5
Please take a look.

File cmd/juju/upgradejuju.go (right):

cmd/juju/upgradejuju.go:175: available, err := envtools.FindTools(env,
client.Major, -1, coretools.Filter{}, envtools.DoNotAllowRetry)
On 2013/11/19 12:37:58, rog wrote:
> // TODO use an API call rather than requiring the environment,
> // so that we can restrict access to the provider secrets
> // while still allowing users to upgrade.
> ?


File state/apiserver/client/client.go (right):

state/apiserver/client/client.go:828: // SetEnvironAgentVersion sets the
environment agent version setting.
On 2013/11/19 12:37:58, rog wrote:
> s/ setting//


File state/apiserver/client/perm_test.go (right):

state/apiserver/client/perm_test.go:429: }
On 2013/11/19 12:37:58, rog wrote:
> On 2013/11/19 11:30:58, jameinel wrote:
> > shouldn't we have an else { unset the version} ?
> > Or we just always assume we have it and we don't use found but just let it
> panic
> > if it isn't there.
> +1
> or get the old version in the code above and return an error if it's not
> tbh all this permission-checking code is past its sell-by date - it was
> when individual operations checked permissions, but we could probably have a
> single call to check that we can't access Client, to replace all these 400
> of gunk. not for this CL though.

agent-version is not set in the scenario that sets up these permission tests. I
initially did exactly what rog's suggested, but it failed with the error "some
agents are not yet running 1.17.0...", when I tried to set it to something else
(e.g. 9.8.7). Moreover, agent-version seems to be missing from the environ
config for these tests, so I cannot get it earlier to restore it later.
And finally, as rog points out these tests should become irrelevant soon as we
go towards user/role based security in more detail.
I just wanted to have a test here that calls SetEnvironAgentVersion, not testing
it fully.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b