cmd/jujud: implement upgrade-juju command

cmd/jujud: implement upgrade-juju command



https://code.launchpad.net/~rogpeppe/juju-core/047-upgrade-juju/+merge/122679

Requires: https://code.launchpad.net/~rogpeppe/juju-core/046-config-dev-version/+merge/122502

(do not edit description out of merge proposal)

[rog, 2012-09-04 14:58:04]

Please take a look.

[niemeyer, 2012-09-05 15:13:32]

https://codereview.appspot.com/6498085/diff/1017/cmd/juju/upgradejuju.go
File cmd/juju/upgradejuju.go (right):

https://codereview.appspot.com/6498085/diff/1017/cmd/juju/upgradejuju.go#newc...
cmd/juju/upgradejuju.go:56: // Run changes the juju-managed firewall to expose
any
Needs fixing.

https://codereview.appspot.com/6498085/diff/1017/cmd/juju/upgradejuju.go#newc...
cmd/juju/upgradejuju.go:87: if c.Version == (version.Number{}) {
IsValid might make sense.

https://codereview.appspot.com/6498085/diff/1017/cmd/juju/upgradejuju.go#newc...
cmd/juju/upgradejuju.go:109: if max != nil {
Taking precedence is fine, but it sounds to me like if we have a newer version
on the public storage we should use it instead of remaining out-of-date and
unable to upgrade.

https://codereview.appspot.com/6498085/diff/1017/cmd/juju/upgradejuju.go#newc...
cmd/juju/upgradejuju.go:132: vers.Minor = (max.Minor / 10000 * 10000) +
(vers.Minor % 10000) + 10000
That's totally wild. It pretty much kills the whole versioning logic once a
single version has been pushed. We'll need something better.

Can we drop the version bumping from this branch so we can push it forward while
we agree on a better scheme?

https://codereview.appspot.com/6498085/diff/1017/cmd/juju/upgradejuju.go#newc...
cmd/juju/upgradejuju.go:137: // highestVersion returns the tools with the
highest
Don't we have this logic implemented inside the environs package?

[rog, 2012-09-06 10:41:10]

Please take a look.

https://codereview.appspot.com/6498085/diff/1017/cmd/juju/upgradejuju.go
File cmd/juju/upgradejuju.go (right):

https://codereview.appspot.com/6498085/diff/1017/cmd/juju/upgradejuju.go#newc...
cmd/juju/upgradejuju.go:56: // Run changes the juju-managed firewall to expose
any
On 2012/09/05 15:13:32, niemeyer wrote:
> Needs fixing.

Done.

https://codereview.appspot.com/6498085/diff/1017/cmd/juju/upgradejuju.go#newc...
cmd/juju/upgradejuju.go:87: if c.Version == (version.Number{}) {
On 2012/09/05 15:13:32, niemeyer wrote:
> IsValid might make sense.

I'm not sure. IsValid is not the same as IsZero. (versions can be invalid in
ways other than being zero, for instance if the version numbers are negative).
I'll reserve doing something along those lines for another CL if that's ok.

https://codereview.appspot.com/6498085/diff/1017/cmd/juju/upgradejuju.go#newc...
cmd/juju/upgradejuju.go:109: if max != nil {
On 2012/09/05 15:13:32, niemeyer wrote:
> Taking precedence is fine, but it sounds to me like if we have a newer version
> on the public storage we should use it instead of remaining out-of-date and
> unable to upgrade.

I considered this, but I think it breaks the simple model that we currently
have. Currently, *any* version in the private storage overrides *any* version in
the public storage. This means that if we upload our local development version
to private storage, we know it will be used.

It's a very simple model. If we say that under some circumstances we will use
the public tools even when there are some private tools available, that model
becomes more complex. In fact, it's worse than that - we're simply proposing a
version here. The clients will follow the usual rule and use the private tools
over the public tools, so they will still not upgrade to the version we've
proposed.

If we want to change the model, we should have a discussion about that.

https://codereview.appspot.com/6498085/diff/1017/cmd/juju/upgradejuju.go#newc...
cmd/juju/upgradejuju.go:132: vers.Minor = (max.Minor / 10000 * 10000) +
(vers.Minor % 10000) + 10000
On 2012/09/05 15:13:32, niemeyer wrote:
> That's totally wild. It pretty much kills the whole versioning logic once a
> single version has been pushed. We'll need something better.
> 
> Can we drop the version bumping from this branch so we can push it forward
while
> we agree on a better scheme?

Done.

https://codereview.appspot.com/6498085/diff/1017/cmd/juju/upgradejuju.go#newc...
cmd/juju/upgradejuju.go:137: // highestVersion returns the tools with the
highest
On 2012/09/05 15:13:32, niemeyer wrote:
> Don't we have this logic implemented inside the environs package?

No. The logic in the environs package finds the highest version that is
compatible with a particular series and architecture. This logic is unusual in
that it doesn't care at all about series and architecture. It's unique enough
that I don't think it warrants a place in the environs logic, but YMMV.

[niemeyer, 2012-09-06 13:02:09]

LGTM

https://codereview.appspot.com/6498085/diff/13001/cmd/juju/upgradejuju.go
File cmd/juju/upgradejuju.go (right):

https://codereview.appspot.com/6498085/diff/13001/cmd/juju/upgradejuju.go#new...
cmd/juju/upgradejuju.go:130: func SetStateAgentVersion(st *state.State, vers
version.Number, development bool) error {
FWIW, SetAgentVersion sounds like a fine name still, even if it's sitting here.

[rog, 2012-09-06 16:38:01]

*** Submitted:

cmd/jujud: implement upgrade-juju command

R=niemeyer
CC=
https://codereview.appspot.com/6498085

https://codereview.appspot.com/6498085/diff/13001/cmd/juju/upgradejuju.go
File cmd/juju/upgradejuju.go (right):

https://codereview.appspot.com/6498085/diff/13001/cmd/juju/upgradejuju.go#new...
cmd/juju/upgradejuju.go:130: func SetStateAgentVersion(st *state.State, vers
version.Number, development bool) error {
On 2012/09/06 13:02:09, niemeyer wrote:
> FWIW, SetAgentVersion sounds like a fine name still, even if it's sitting
here.

Done.