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

Issue 8540050: cmd/juju: upgrade-charm --switch support (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 7 months ago by dimitern
Modified:
8 years, 7 months ago
Reviewers:
mp+160910
Visibility:
Public.

Description

cmd/juju: upgrade-charm --switch support This implements a new argument to upgrade-charm: --switch <charm-url>. The passed URL is inferred to get the complete URL and used instead of the service's newest charm url revision. Also --revision is now supported, to give an explicit revision to upgrade to, rather than the latest. There are a few related sanity checks that need to be implemented as well (in follow-ups), so we can better check charm compatibility before upgrading. Fixes bugs #1040210 and #1050750. https://code.launchpad.net/~dimitern/juju-core/038-upgrade-charm-switch/+merge/160910 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 28

Patch Set 2 : cmd/juju: upgrade-charm --switch support #

Total comments: 8

Patch Set 3 : cmd/juju: upgrade-charm --switch support #

Patch Set 4 : cmd/juju: upgrade-charm --switch support #

Patch Set 5 : cmd/juju: upgrade-charm --switch support #

Total comments: 25

Patch Set 6 : cmd/juju: upgrade-charm --switch support #

Patch Set 7 : cmd/juju: upgrade-charm --switch support #

Total comments: 6

Patch Set 8 : cmd/juju: upgrade-charm --switch support #

Total comments: 2

Patch Set 9 : cmd/juju: upgrade-charm --switch support #

Unified diffs Side-by-side diffs Delta from patch set Stats (+183 lines, -38 lines) Patch
A [revision details] View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/upgradecharm.go View 1 2 3 4 5 6 7 8 4 chunks +88 lines, -29 lines 0 comments Download
M cmd/juju/upgradecharm_test.go View 1 2 3 4 5 6 7 8 6 chunks +84 lines, -9 lines 0 comments Download
M testing/charm.go View 1 1 chunk +9 lines, -0 lines 0 comments Download

Messages

Total messages: 21
dimitern
Please take a look.
8 years, 7 months ago (2013-04-25 14:33:14 UTC) #1
TheMue
LGTM with one question. https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm_test.go File cmd/juju/upgradecharm_test.go (right): https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm_test.go#newcode107 cmd/juju/upgradecharm_test.go:107: func (s *UpgradeCharmSuccessSuite) assertLocalRevision(c *C, ...
8 years, 7 months ago (2013-04-25 14:40:06 UTC) #2
rog
good direction, but does not LGTM quite yet - some logic i don't think is ...
8 years, 7 months ago (2013-04-25 14:43:52 UTC) #3
fwereade
a few thoughts https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go File cmd/juju/upgradecharm.go (left): https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#oldcode67 cmd/juju/upgradecharm.go:67: // TODO(dimitern): add the other flags ...
8 years, 7 months ago (2013-04-25 15:09:06 UTC) #4
dimitern
Nothing changed yet, I just need to make sure I get the suggestions first. https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go ...
8 years, 7 months ago (2013-04-25 15:29:01 UTC) #5
fwereade
https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go File cmd/juju/upgradecharm.go (left): https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#oldcode67 cmd/juju/upgradecharm.go:67: // TODO(dimitern): add the other flags --switch and --revision. ...
8 years, 7 months ago (2013-04-25 15:43:30 UTC) #6
dimitern
Please take a look. https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go File cmd/juju/upgradecharm.go (right): https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcode38 cmd/juju/upgradecharm.go:38: Note that the given charm ...
8 years, 7 months ago (2013-04-26 14:29:01 UTC) #7
rog
still not quite there i'm afraid. https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go File cmd/juju/upgradecharm.go (right): https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go#newcode46 cmd/juju/upgradecharm.go:46: as the old ...
8 years, 7 months ago (2013-04-26 15:06:32 UTC) #8
dimitern
Please take a look. https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go File cmd/juju/upgradecharm.go (right): https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go#newcode46 cmd/juju/upgradecharm.go:46: as the old charm. On ...
8 years, 7 months ago (2013-04-26 16:34:42 UTC) #9
dimitern
Sorry, forgot to reply to these - all done. https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go File cmd/juju/upgradecharm.go (right): https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go#newcode123 cmd/juju/upgradecharm.go:123: ...
8 years, 7 months ago (2013-04-26 16:37:19 UTC) #10
dimitern
Please take a look.
8 years, 7 months ago (2013-04-26 17:50:33 UTC) #11
dimitern
Please take a look.
8 years, 7 months ago (2013-04-26 17:57:11 UTC) #12
fwereade
https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go File cmd/juju/upgradecharm.go (left): https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#oldcode66 cmd/juju/upgradecharm.go:66: } if c.SwitchURL != "" && c.Revision != -1 ...
8 years, 7 months ago (2013-04-28 10:41:25 UTC) #13
dimitern
Please take a look. https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go File cmd/juju/upgradecharm.go (left): https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#oldcode66 cmd/juju/upgradecharm.go:66: } On 2013/04/28 10:41:25, fwereade ...
8 years, 7 months ago (2013-04-29 09:53:21 UTC) #14
fwereade
I'm on the fence a little; let's have a chat about how bad the revision-bumping ...
8 years, 7 months ago (2013-04-29 10:45:26 UTC) #15
dimitern
Please take a look. https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go File cmd/juju/upgradecharm.go (left): https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#oldcode95 cmd/juju/upgradecharm.go:95: return fmt.Errorf("already running latest charm ...
8 years, 7 months ago (2013-04-29 12:05:56 UTC) #16
fwereade
nearly there, last round I think https://codereview.appspot.com/8540050/diff/32001/cmd/juju/upgradecharm.go File cmd/juju/upgradecharm.go (left): https://codereview.appspot.com/8540050/diff/32001/cmd/juju/upgradecharm.go#oldcode100 cmd/juju/upgradecharm.go:100: } else if ...
8 years, 7 months ago (2013-04-29 15:45:18 UTC) #17
rog
really close. just one final piece of logic to get right, i think. https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go File ...
8 years, 7 months ago (2013-04-29 16:11:50 UTC) #18
dimitern
Please take a look.
8 years, 7 months ago (2013-04-29 16:30:09 UTC) #19
fwereade
LGTM with suggestions https://codereview.appspot.com/8540050/diff/25002/cmd/juju/upgradecharm.go File cmd/juju/upgradecharm.go (left): https://codereview.appspot.com/8540050/diff/25002/cmd/juju/upgradecharm.go#oldcode100 cmd/juju/upgradecharm.go:100: } else if _, bumpRevision = ...
8 years, 7 months ago (2013-04-29 16:34:08 UTC) #20
dimitern
8 years, 7 months ago (2013-04-29 16:56:04 UTC) #21
*** Submitted:

cmd/juju: upgrade-charm --switch support

This implements a new argument to upgrade-charm:
--switch <charm-url>. The passed URL is inferred
to get the complete URL and used instead of the
service's newest charm url revision.
Also --revision is now supported, to give an
explicit revision to upgrade to, rather than the
latest.

There are a few related sanity checks that need
to be implemented as well (in follow-ups), so we
can better check charm compatibility before upgrading.

Fixes bugs #1040210 and #1050750.

R=TheMue, rog, fwereade
CC=
https://codereview.appspot.com/8540050
Sign in to reply to this message.

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