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

Issue 25080043: cmd/juju: upgrade-juju minor version upgrades (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years ago by dimitern
Modified:
6 years ago
Reviewers:
mp+194822, fwereade, rog
Visibility:
Public.

Description

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)

Patch Set 1 #

Total comments: 24

Patch Set 2 : cmd/juju: upgrade-juju minor version upgrades #

Total comments: 10

Patch Set 3 : cmd/juju: upgrade-juju minor version upgrades #

Total comments: 8

Patch Set 4 : cmd/juju: upgrade-juju minor version upgrades #

Unified diffs Side-by-side diffs Delta from patch set Stats (+203 lines, -122 lines) Patch
A [revision details] View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/upgradejuju.go View 1 2 3 7 chunks +61 lines, -39 lines 0 comments Download
M cmd/juju/upgradejuju_test.go View 1 2 3 9 chunks +49 lines, -63 lines 0 comments Download
M tools/list.go View 1 2 3 1 chunk +20 lines, -0 lines 0 comments Download
M tools/list_test.go View 1 2 3 8 chunks +71 lines, -20 lines 0 comments Download

Messages

Total messages: 8
dimitern
Please take a look.
6 years ago (2013-11-12 10:39:18 UTC) #1
fwereade
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#oldcode250 cmd/juju/upgradejuju.go:250: } I'm finding this new chunk hard to follow ...
6 years ago (2013-11-12 17:04:39 UTC) #2
rog
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). ...
6 years ago (2013-11-12 17:09:37 UTC) #3
dimitern
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#oldcode250 cmd/juju/upgradejuju.go:250: } On 2013/11/12 17:04:39, fwereade ...
6 years ago (2013-11-13 12:18:57 UTC) #4
fwereade
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#newcode262 ...
6 years ago (2013-11-13 13:02:49 UTC) #5
dimitern
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#newcode262 cmd/juju/upgradejuju.go:262: } else { On 2013/11/13 ...
6 years ago (2013-11-13 14:05:14 UTC) #6
fwereade
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#newcode256 cmd/juju/upgradejuju.go:256: ...
6 years ago (2013-11-14 08:45:46 UTC) #7
dimitern
6 years ago (2013-11-14 09:12:10 UTC) #8
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 :)
Sign in to reply to this message.

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