cmd/juju: upgrade-juju minor version upgrades

cmd/juju: upgrade-juju minor version upgrades

This changes a few things needed to fix lp:1233451:
 * --dev flag is removed, as the logic changed, see
   below;
 * default behavior, when --version is not specified
   now tries to upgrade to the *next* stable (major.minor+2)
   version first, failing that will try the most recent
   *current* version;
 * development flags in the environment config, or the
   client's development version is not used to decide
   whether to pick a development version for upgrade:
   the only way to upgrade to a development version now
   is by using --version only (even if the current one
   is dev);
 * improved tests, removed the (not redundant) public
   storage, as it's not used anymore;
 * explicit --version overrides the behavior described
   above and effectively forces and upgrade to the given
   version, regardless if it's supported (no warning).
 * when changing the agent-version in the environment,
   the new SetEnvironAgentVersion is used, to ensure
   all agents are running the same version first, so
   that partial upgrades will be detected and reported.

All this is needed to ensure we can do proper upgrades
to the next/current supported version properly and is
required before we can convert upgrade-juju to use the API.

https://code.launchpad.net/~dimitern/juju-core/201-lp-1233451-minor-version-upgrades/+merge/194822

(do not edit description out of merge proposal)

[dimitern, 2013-11-12 10:39:18]

Please take a look.

[fwereade, 2013-11-12 17:04:39]

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go
File cmd/juju/upgradejuju.go (left):

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#oldcod...
cmd/juju/upgradejuju.go:250: }
I'm finding this new chunk hard to follow -- can we maybe define some
tools-filtering stuff that makes it easier to express what you're doing here, as
a step towards saner long-term behaviour as discussed in chat today? Given that,
I think I'd be happy to accept this CL as progress, if not perfection ;).

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

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode46
cmd/juju/upgradejuju.go:46: major.minor is given (e.g. --version 1.17, or
1.17.2, but not just 1)
I do still think that matching major.minor is good enough (for x.odd) -- or
major.minor.patch.build for x.y.z[.a]. That is, if you're already dev, you can
upgrade automatically to a closely-matching version.

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode59
cmd/juju/upgradejuju.go:59: Any version with an even minor version number is a
stable version.
Stick this line up with the definition of "development".

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode60
cmd/juju/upgradejuju.go:60: - The highest patch.build version of the *current*
major.minor version.
Is the patch.build stuff right? I think there's a potential problem if we allow
.build from non-.build versions...

This of course is complicated to hell and back by --upload-tools... although
maybe just fixing the prev para, to point out explicitly that uploaded tools
will always be marked as dev, will be enough for clarity..?

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode64
cmd/juju/upgradejuju.go:64: you manage yourself; see the documentation for
"sync-tools".
Huh. Does maas require sync-tools? People might legitimately want to do so,
sure, but I think the only reason one would have to is... oh, blast. We're not
using streams.canonical.com yet, are we? OK, this is accurate but will *become*
inaccurate soon.

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode69
cmd/juju/upgradejuju.go:69: but 1.16 -> 2.0 is not officially supported,
although it might work).
This feels a bit mistargeted -- it's dev instruction, not user documentation.

[rog, 2013-11-12 17:09:37]

Looks generally good with a few remarks below.

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

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode40
cmd/juju/upgradejuju.go:40: %s).
The convenience of seeing the current version in the help output
seems to me to be outweighed by the fragility of passing the
entire thing through Sprintf. I'd lose it.

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode46
cmd/juju/upgradejuju.go:46: major.minor is given (e.g. --version 1.17, or
1.17.2, but not just 1)
On 2013/11/12 17:04:39, fwereade wrote:
> I do still think that matching major.minor is good enough (for x.odd) -- or
> major.minor.patch.build for x.y.z[.a]. That is, if you're already dev, you can
> upgrade automatically to a closely-matching version.

+1

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode59
cmd/juju/upgradejuju.go:59: Any version with an even minor version number is a
stable version.
Rather than defining both "development" and "stable" as opposites to one
another, I'd prefer if one term was used throughout.
Also, "patch" and "build" aren't defined at all above,
and I know that I for one was not familiar with the
all the terms for each component of the version.

That is, either define everything above in terms of "stable" versions, or define
this clause in terms of "non-development" version,

I wonder if we could usefully leave things a little less fully specified here to
make things easier for the user reading the docs.

For example:

When run without arguments, upgrade-juju will try to upgrade
to the next available non-development version.

I don't think we necessarily need to describe what "next" means 
in that much detail.

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode66
cmd/juju/upgradejuju.go:66: The last stable release of a major version series,
must be able to upgrade
This paragraph sounds like developer documentation to me.

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcod...
cmd/juju/upgradejuju.go:246: // and ensures the tools list contains no entries
that do not have that version.
s/tools list/v.tools/

?
(I spent a few moments wondering why the code was side-effecting v.tools)

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcod...
cmd/juju/upgradejuju.go:255: if v.agent.IsDev() {
or

nextStable.Minor = (nextStable.Minor &^ 1) + 2

but perhaps that's less clear.

[dimitern, 2013-11-13 12:18:57]

Please take a look.

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go
File cmd/juju/upgradejuju.go (left):

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#oldcod...
cmd/juju/upgradejuju.go:250: }
On 2013/11/12 17:04:39, fwereade wrote:
> I'm finding this new chunk hard to follow -- can we maybe define some
> tools-filtering stuff that makes it easier to express what you're doing here,
as
> a step towards saner long-term behaviour as discussed in chat today? Given
that,
> I think I'd be happy to accept this CL as progress, if not perfection ;).

Added tools.List.NewestOf(base) method, which significantly simplifies the whole
chunk above.

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

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode40
cmd/juju/upgradejuju.go:40: %s).
On 2013/11/12 17:09:37, rog wrote:
> The convenience of seeing the current version in the help output
> seems to me to be outweighed by the fragility of passing the
> entire thing through Sprintf. I'd lose it.

Done.

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode46
cmd/juju/upgradejuju.go:46: major.minor is given (e.g. --version 1.17, or
1.17.2, but not just 1)
On 2013/11/12 17:04:39, fwereade wrote:
> I do still think that matching major.minor is good enough (for x.odd) -- or
> major.minor.patch.build for x.y.z[.a]. That is, if you're already dev, you can
> upgrade automatically to a closely-matching version.

Done.

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode46
cmd/juju/upgradejuju.go:46: major.minor is given (e.g. --version 1.17, or
1.17.2, but not just 1)
On 2013/11/12 17:09:37, rog wrote:
> On 2013/11/12 17:04:39, fwereade wrote:
> > I do still think that matching major.minor is good enough (for x.odd) -- or
> > major.minor.patch.build for x.y.z[.a]. That is, if you're already dev, you
can
> > upgrade automatically to a closely-matching version.
> 
> +1

Done.

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode59
cmd/juju/upgradejuju.go:59: Any version with an even minor version number is a
stable version.
On 2013/11/12 17:04:39, fwereade wrote:
> Stick this line up with the definition of "development".

Removed, since we already define what a development version is above.

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode59
cmd/juju/upgradejuju.go:59: Any version with an even minor version number is a
stable version.
On 2013/11/12 17:09:37, rog wrote:
> Rather than defining both "development" and "stable" as opposites to one
> another, I'd prefer if one term was used throughout.
> Also, "patch" and "build" aren't defined at all above,
> and I know that I for one was not familiar with the
> all the terms for each component of the version.
> 
> That is, either define everything above in terms of "stable" versions, or
define
> this clause in terms of "non-development" version,
> 
> I wonder if we could usefully leave things a little less fully specified here
to
> make things easier for the user reading the docs.
> 
> For example:
> 
> When run without arguments, upgrade-juju will try to upgrade
> to the next available non-development version.
> 
> I don't think we necessarily need to describe what "next" means 
> in that much detail.

I think describing what we mean by "next" is useful here. Also, the proposed
statement is not entirely correct, because we will upgrade to a more recent
(current) development version, if the current one is dev.

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode60
cmd/juju/upgradejuju.go:60: - The highest patch.build version of the *current*
major.minor version.
On 2013/11/12 17:04:39, fwereade wrote:
> Is the patch.build stuff right? I think there's a potential problem if we
allow
> .build from non-.build versions...
> 
> This of course is complicated to hell and back by --upload-tools... although
> maybe just fixing the prev para, to point out explicitly that uploaded tools
> will always be marked as dev, will be enough for clarity..?

Current major.minor is now preferred when a more recent patch.build is
available, even if a stable version is available as well.

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode64
cmd/juju/upgradejuju.go:64: you manage yourself; see the documentation for
"sync-tools".
On 2013/11/12 17:04:39, fwereade wrote:
> Huh. Does maas require sync-tools? People might legitimately want to do so,
> sure, but I think the only reason one would have to is... oh, blast. We're not
> using http://streams.canonical.com yet, are we? OK, this is accurate but will
*become*
> inaccurate soon.

Not sure if we need sync-tools for maas. Anyhow, when that changes we'll need to
revise the above statement.

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode66
cmd/juju/upgradejuju.go:66: The last stable release of a major version series,
must be able to upgrade
On 2013/11/12 17:09:37, rog wrote:
> This paragraph sounds like developer documentation to me.

Removed.

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcode69
cmd/juju/upgradejuju.go:69: but 1.16 -> 2.0 is not officially supported,
although it might work).
On 2013/11/12 17:04:39, fwereade wrote:
> This feels a bit mistargeted -- it's dev instruction, not user documentation.

Removed.

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcod...
cmd/juju/upgradejuju.go:246: // and ensures the tools list contains no entries
that do not have that version.
On 2013/11/12 17:09:37, rog wrote:
> s/tools list/v.tools/
> 
> ?
> (I spent a few moments wondering why the code was side-effecting v.tools)

v.tools is not changed anymore in the new code.

https://codereview.appspot.com/25080043/diff/1/cmd/juju/upgradejuju.go#newcod...
cmd/juju/upgradejuju.go:255: if v.agent.IsDev() {
On 2013/11/12 17:09:37, rog wrote:
> or
> 
> nextStable.Minor = (nextStable.Minor &^ 1) + 2
> 
> but perhaps that's less clear.

It's definitely less clear, but with the changes now it should be more obvious
what we're doing here.

[fwereade, 2013-11-13 13:02:49]

Nearly there, a couple of minors still to tidy up.

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

https://codereview.appspot.com/25080043/diff/20001/cmd/juju/upgradejuju.go#ne...
cmd/juju/upgradejuju.go:262: } else {
I'd find this easier to follow if the condition were inverted -- while == tests
are often nicer than != tests, I think the logic flows more naturally with a !=
here.

https://codereview.appspot.com/25080043/diff/20001/cmd/juju/upgradejuju_test.go
File cmd/juju/upgradejuju_test.go (right):

https://codereview.appspot.com/25080043/diff/20001/cmd/juju/upgradejuju_test....
cmd/juju/upgradejuju_test.go:51: about:          "deprecated --dev specified",
technically I think that no longer qualifies as "deprecated"

https://codereview.appspot.com/25080043/diff/20001/tools/list.go
File tools/list.go (right):

https://codereview.appspot.com/25080043/diff/20001/tools/list.go#newcode90
tools/list.go:90: // patch and build numbers.
Don't really like that name. NewestMatching? NewestCompatible?

https://codereview.appspot.com/25080043/diff/20001/tools/list.go#newcode91
tools/list.go:91: func (src List) NewestOf(base version.Number) version.Number {
A comma-ok return rather than a zero would be nice here -- I'd really like to
avoid in-band signalling.

https://codereview.appspot.com/25080043/diff/20001/tools/list_test.go
File tools/list_test.go (left):

https://codereview.appspot.com/25080043/diff/20001/tools/list_test.go#oldcode52
tools/list_test.go:52: tAll         = extend(t100all, t190all, append(t200all,
t2001precise))
tAll is no longer tAll ;p.

[dimitern, 2013-11-13 14:05:14]

Please take a look.

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

https://codereview.appspot.com/25080043/diff/20001/cmd/juju/upgradejuju.go#ne...
cmd/juju/upgradejuju.go:262: } else {
On 2013/11/13 13:02:49, fwereade wrote:
> I'd find this easier to follow if the condition were inverted -- while ==
tests
> are often nicer than != tests, I think the logic flows more naturally with a
!=
> here.

Done.

https://codereview.appspot.com/25080043/diff/20001/cmd/juju/upgradejuju_test.go
File cmd/juju/upgradejuju_test.go (right):

https://codereview.appspot.com/25080043/diff/20001/cmd/juju/upgradejuju_test....
cmd/juju/upgradejuju_test.go:51: about:          "deprecated --dev specified",
On 2013/11/13 13:02:49, fwereade wrote:
> technically I think that no longer qualifies as "deprecated"

Changed to "removed".

https://codereview.appspot.com/25080043/diff/20001/tools/list.go
File tools/list.go (right):

https://codereview.appspot.com/25080043/diff/20001/tools/list.go#newcode90
tools/list.go:90: // patch and build numbers.
On 2013/11/13 13:02:49, fwereade wrote:
> Don't really like that name. NewestMatching? NewestCompatible?

NewestCompatible it is.

https://codereview.appspot.com/25080043/diff/20001/tools/list.go#newcode91
tools/list.go:91: func (src List) NewestOf(base version.Number) version.Number {
On 2013/11/13 13:02:49, fwereade wrote:
> A comma-ok return rather than a zero would be nice here -- I'd really like to
> avoid in-band signalling.

Done.

https://codereview.appspot.com/25080043/diff/20001/tools/list_test.go
File tools/list_test.go (left):

https://codereview.appspot.com/25080043/diff/20001/tools/list_test.go#oldcode52
tools/list_test.go:52: tAll         = extend(t100all, t190all, append(t200all,
t2001precise))
On 2013/11/13 13:02:49, fwereade wrote:
> tAll is no longer tAll ;p.

Yeah, but I didn't want to disturb all other tests by adding t210all there or
renaming it to something like tAllExcept210 :)

[fwereade, 2013-11-14 08:45:46]

LGTM with agreement on priority, and the other trivials.

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

https://codereview.appspot.com/25080043/diff/40001/cmd/juju/upgradejuju.go#ne...
cmd/juju/upgradejuju.go:256: v.chosen = newestCurrent
Shouldn't we prioritise nextStable over newestCurrent?

https://codereview.appspot.com/25080043/diff/40001/cmd/juju/upgradejuju.go#ne...
cmd/juju/upgradejuju.go:260: log.Debugf("found a supported more recent stable
version %s", newestNextStable)
supported and stable are redundant, I think -- drop "supported"?

https://codereview.appspot.com/25080043/diff/40001/tools/list.go
File tools/list.go (right):

https://codereview.appspot.com/25080043/diff/40001/tools/list.go#newcode88
tools/list.go:88: // NewestOf returns the most recent version compatible with
base,
s/Of/Compatible/

https://codereview.appspot.com/25080043/diff/40001/tools/list_test.go
File tools/list_test.go (left):

https://codereview.appspot.com/25080043/diff/40001/tools/list_test.go#oldcode52
tools/list_test.go:52: tAll         = extend(t100all, t190all, append(t200all,
t2001precise))
Can we make it tAllBefore210 or something please?

http://thecodelesscode.com/case/84 ;p

[dimitern, 2013-11-14 09:12:10]

Please take a look.

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

https://codereview.appspot.com/25080043/diff/40001/cmd/juju/upgradejuju.go#ne...
cmd/juju/upgradejuju.go:256: v.chosen = newestCurrent
On 2013/11/14 08:45:46, fwereade wrote:
> Shouldn't we prioritise nextStable over newestCurrent?

As discussed online. Done.

https://codereview.appspot.com/25080043/diff/40001/cmd/juju/upgradejuju.go#ne...
cmd/juju/upgradejuju.go:260: log.Debugf("found a supported more recent stable
version %s", newestNextStable)
On 2013/11/14 08:45:46, fwereade wrote:
> supported and stable are redundant, I think -- drop "supported"?

Done.

https://codereview.appspot.com/25080043/diff/40001/tools/list.go
File tools/list.go (right):

https://codereview.appspot.com/25080043/diff/40001/tools/list.go#newcode88
tools/list.go:88: // NewestOf returns the most recent version compatible with
base,
On 2013/11/14 08:45:46, fwereade wrote:
> s/Of/Compatible/

Done.

https://codereview.appspot.com/25080043/diff/40001/tools/list_test.go
File tools/list_test.go (left):

https://codereview.appspot.com/25080043/diff/40001/tools/list_test.go#oldcode52
tools/list_test.go:52: tAll         = extend(t100all, t190all, append(t200all,
t2001precise))
On 2013/11/14 08:45:46, fwereade wrote:
> Can we make it tAllBefore210 or something please?
> 
> http://thecodelesscode.com/case/84 ;p

Ok, good point :)