cmd/juju: upgrade-juju uses the API

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)

[dimitern, 2013-11-18 16:15:25]

Please take a look.

[axw, 2013-11-19 09:37:55]

On 2013/11/18 16:15:25, dimitern wrote:
> Please take a look.

LGTM

[jameinel, 2013-11-19 11:30:58]

LGTM

I think this change is ok as is, though I'd like us to keep working on
upgrade-juju in the long run.

Specifically, the #1 goal of CLI API is to get us to not having direct state
access, and this fully complies to that.

I think we want UpgradeJuju to be done more inside the API server than it is
today, but we can iterate there rather than still having DB access.

https://codereview.appspot.com/21940044/diff/1/state/apiserver/client/perm_te...
File state/apiserver/client/perm_test.go (right):

https://codereview.appspot.com/21940044/diff/1/state/apiserver/client/perm_te...
state/apiserver/client/perm_test.go:429: }
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.

[rog, 2013-11-19 12:37:58]

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#newcod...
cmd/juju/upgradejuju.go:175: available, err := envtools.FindTools(env,
client.Major, -1, coretools.Filter{}, envtools.DoNotAllowRetry)
// 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.

?

https://codereview.appspot.com/21940044/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/21940044/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:828: // SetEnvironAgentVersion sets the
environment agent version setting.
s/ setting//

https://codereview.appspot.com/21940044/diff/1/state/apiserver/client/perm_te...
File state/apiserver/client/perm_test.go (right):

https://codereview.appspot.com/21940044/diff/1/state/apiserver/client/perm_te...
state/apiserver/client/perm_test.go:429: }
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 there.

tbh all this permission-checking code is past its sell-by date - it was relevant
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 lines
of gunk. not for this CL though.

[dimitern, 2013-11-19 16:15:27]

Please take a look.

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#newcod...
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.
> 
> ?

Done.

https://codereview.appspot.com/21940044/diff/1/state/apiserver/client/client.go
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/21940044/diff/1/state/apiserver/client/client....
state/apiserver/client/client.go:828: // SetEnvironAgentVersion sets the
environment agent version setting.
On 2013/11/19 12:37:58, rog wrote:
> s/ setting//

Done.

https://codereview.appspot.com/21940044/diff/1/state/apiserver/client/perm_te...
File state/apiserver/client/perm_test.go (right):

https://codereview.appspot.com/21940044/diff/1/state/apiserver/client/perm_te...
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
there.
> 
> tbh all this permission-checking code is past its sell-by date - it was
relevant
> 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
lines
> 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.