cmd/juju: upgrade-charm --switch support

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)

[dimitern, 2013-04-25 14:33:14]

Please take a look.

[TheMue, 2013-04-25 14:40:06]

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#n...
cmd/juju/upgradecharm_test.go:107: func (s *UpgradeCharmSuccessSuite)
assertLocalRevision(c *C, revision int, path string) {
Maybe I'm only blind or it is planned for later, but where are you using this
method with a different path than s.path?

[rog, 2013-04-25 14:43:52]

good direction, but does not LGTM quite yet - some logic i don't think is quite
right, as outlined below.

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 must be compatible with
the current one.
Perhaps say what "compatible" means in this context in a concise and easy to
understand way (no need to go into all the details, but an overview would be
good so the user knows what they're up against)

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcod...
cmd/juju/upgradecharm.go:106: rev, err := repo.Latest(curl)
i think this is wrong in the context of SwitchURL.

i think the Latest and bump-revision logic should only be triggered when the
charm url does not have a revision number specified.

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#n...
cmd/juju/upgradecharm_test.go:179: s.assertLocalRevision(c, 7, myriakPath)
also test switching from one cs: charm to another?
and with a revision number explicitly specified.

https://codereview.appspot.com/8540050/diff/1/testing/charm.go
File testing/charm.go (right):

https://codereview.appspot.com/8540050/diff/1/testing/charm.go#newcode62
testing/charm.go:62: check(exec.Command("mv", newDst, renamedDst).Run())
os.Rename(newDst, renamedDst) ?

[fwereade, 2013-04-25 15:09:06]

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 --switch and
--revision.
I don't see an implementation of --revision, and I think it'd be quite easy to
sneak it into this CL ;)

(consider that --switch and --revision could in theory conflict, so it might be
good to barf here if they're both set)

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 must be compatible with
the current one.
On 2013/04/25 14:43:52, rog wrote:
> Perhaps say what "compatible" means in this context in a concise and easy to
> understand way (no need to go into all the details, but an overview would be
> good so the user knows what they're up against)

and underscore that it's dangerous -- we can only check so much compatibility :)

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcod...
cmd/juju/upgradecharm.go:100: curl, _ = service.CharmURL()
wrt rog's comment below, curl = curl.WithRevision(-1)

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcod...
cmd/juju/upgradecharm.go:106: rev, err := repo.Latest(curl)
On 2013/04/25 14:43:52, rog wrote:
> i think this is wrong in the context of SwitchURL.
> 
> i think the Latest and bump-revision logic should only be triggered when the
> charm url does not have a revision number specified.

+1

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#n...
cmd/juju/upgradecharm_test.go:94: func (s *UpgradeCharmSuccessSuite)
assertUpgraded(c *C, revision int, forced bool, curl *charm.URL) {
I'd rather return the charm url and assert it in the places it's needed, I think

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm_test.go#n...
cmd/juju/upgradecharm_test.go:179: s.assertLocalRevision(c, 7, myriakPath)
On 2013/04/25 14:43:52, rog wrote:
> also test switching from one cs: charm to another?
> and with a revision number explicitly specified.

We have thus far gone with the shared agreement that if it works with one repo
(local), we can expect it to work with the other. Hmm, can we now fake out the
charm store? I think we can. If so, definite +1 on testing this in a wider range
of scenarios.

https://codereview.appspot.com/8540050/diff/1/testing/charm.go
File testing/charm.go (right):

https://codereview.appspot.com/8540050/diff/1/testing/charm.go#newcode62
testing/charm.go:62: check(exec.Command("mv", newDst, renamedDst).Run())
On 2013/04/25 14:43:52, rog wrote:
> os.Rename(newDst, renamedDst) ?

+1

[dimitern, 2013-04-25 15:29:01]

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
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.
On 2013/04/25 15:09:06, fwereade wrote:
> I don't see an implementation of --revision, and I think it'd be quite easy to
> sneak it into this CL ;)
> 
> (consider that --switch and --revision could in theory conflict, so it might
be
> good to barf here if they're both set)

So --revision is an int and forces the revision as specified, but only when
--switch is not present, otherwise - barf?

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 must be compatible with
the current one.
On 2013/04/25 15:09:06, fwereade wrote:
> On 2013/04/25 14:43:52, rog wrote:
> > Perhaps say what "compatible" means in this context in a concise and easy to
> > understand way (no need to go into all the details, but an overview would be
> > good so the user knows what they're up against)
> 
> and underscore that it's dangerous -- we can only check so much compatibility
:)

I'd appreciate suggestions with actual text, since probably both of you can
devise a better sentence than me :)

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcod...
cmd/juju/upgradecharm.go:100: curl, _ = service.CharmURL()
On 2013/04/25 15:09:06, fwereade wrote:
> wrt rog's comment below, curl = curl.WithRevision(-1)

When --switch is given, the revision shouldn't be bumped, and the code below
does not apply, right?

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcod...
cmd/juju/upgradecharm.go:106: rev, err := repo.Latest(curl)
On 2013/04/25 15:09:06, fwereade wrote:
> On 2013/04/25 14:43:52, rog wrote:
> > i think this is wrong in the context of SwitchURL.
> > 
> > i think the Latest and bump-revision logic should only be triggered when the
> > charm url does not have a revision number specified.
> 
> +1

See the question above.

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#n...
cmd/juju/upgradecharm_test.go:94: func (s *UpgradeCharmSuccessSuite)
assertUpgraded(c *C, revision int, forced bool, curl *charm.URL) {
On 2013/04/25 15:09:06, fwereade wrote:
> I'd rather return the charm url and assert it in the places it's needed, I
think

Good point, will do.

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm_test.go#n...
cmd/juju/upgradecharm_test.go:107: func (s *UpgradeCharmSuccessSuite)
assertLocalRevision(c *C, revision int, path string) {
On 2013/04/25 14:40:06, TheMue wrote:
> Maybe I'm only blind or it is planned for later, but where are you using this
> method with a different path than s.path?

Only in the last test case path != s.path.

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm_test.go#n...
cmd/juju/upgradecharm_test.go:179: s.assertLocalRevision(c, 7, myriakPath)
On 2013/04/25 15:09:06, fwereade wrote:
> On 2013/04/25 14:43:52, rog wrote:
> > also test switching from one cs: charm to another?
> > and with a revision number explicitly specified.
> 
> We have thus far gone with the shared agreement that if it works with one repo
> (local), we can expect it to work with the other. Hmm, can we now fake out the
> charm store? I think we can. If so, definite +1 on testing this in a wider
range
> of scenarios.

Sorry, I have no clue how to test this - ideas?

https://codereview.appspot.com/8540050/diff/1/testing/charm.go
File testing/charm.go (right):

https://codereview.appspot.com/8540050/diff/1/testing/charm.go#newcode62
testing/charm.go:62: check(exec.Command("mv", newDst, renamedDst).Run())
On 2013/04/25 15:09:06, fwereade wrote:
> On 2013/04/25 14:43:52, rog wrote:
> > os.Rename(newDst, renamedDst) ?
> 
> +1

Will do.

[fwereade, 2013-04-25 15:43:30]

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.
On 2013/04/25 15:29:01, dimitern wrote:
> On 2013/04/25 15:09:06, fwereade wrote:
> > I don't see an implementation of --revision, and I think it'd be quite easy
to
> > sneak it into this CL ;)
> > 
> > (consider that --switch and --revision could in theory conflict, so it might
> be
> > good to barf here if they're both set)
> 
> So --revision is an int and forces the revision as specified, but only when
> --switch is not present, otherwise - barf?

Yeah, I think that's sane.

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#newcod...
cmd/juju/upgradecharm.go:100: curl, _ = service.CharmURL()
On 2013/04/25 15:29:01, dimitern wrote:
> On 2013/04/25 15:09:06, fwereade wrote:
> > wrt rog's comment below, curl = curl.WithRevision(-1)
> 
> When --switch is given, the revision shouldn't be bumped, and the code below
> does not apply, right?

Hmm.I suspect that bump-revision logic *should* apply when --switch is given
with a *local* charm url *without* an explicit revision. Sane?

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcod...
cmd/juju/upgradecharm.go:106: rev, err := repo.Latest(curl)
On 2013/04/25 15:29:01, dimitern wrote:
> See the question above.

So I think it's:

rev := curl.Revision
if rev == -1 {
    latest, err := repo.Latest(curl)
    if err != nil {
        return err
    }
    rev = latest
}

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#n...
cmd/juju/upgradecharm_test.go:179: s.assertLocalRevision(c, 7, myriakPath)
On 2013/04/25 15:29:01, dimitern wrote:
> On 2013/04/25 15:09:06, fwereade wrote:
> > On 2013/04/25 14:43:52, rog wrote:
> > > also test switching from one cs: charm to another?
> > > and with a revision number explicitly specified.
> > 
> > We have thus far gone with the shared agreement that if it works with one
repo
> > (local), we can expect it to work with the other. Hmm, can we now fake out
the
> > charm store? I think we can. If so, definite +1 on testing this in a wider
> range
> > of scenarios.
> 
> Sorry, I have no clue how to test this - ideas?

I'd be fine with setting charm.Store to some other repo and checking that it
talks properly to what it *thinks* is the store

[dimitern, 2013-04-26 14:29:01]

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 must be compatible with
the current one.
On 2013/04/25 15:29:01, dimitern wrote:
> On 2013/04/25 15:09:06, fwereade wrote:
> > On 2013/04/25 14:43:52, rog wrote:
> > > Perhaps say what "compatible" means in this context in a concise and easy
to
> > > understand way (no need to go into all the details, but an overview would
be
> > > good so the user knows what they're up against)
> > 
> > and underscore that it's dangerous -- we can only check so much
compatibility
> :)
> 
> I'd appreciate suggestions with actual text, since probably both of you can
> devise a better sentence than me :)

Please check the new wording.

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcod...
cmd/juju/upgradecharm.go:100: curl, _ = service.CharmURL()
On 2013/04/25 15:43:30, fwereade wrote:
> On 2013/04/25 15:29:01, dimitern wrote:
> > On 2013/04/25 15:09:06, fwereade wrote:
> > > wrt rog's comment below, curl = curl.WithRevision(-1)
> > 
> > When --switch is given, the revision shouldn't be bumped, and the code below
> > does not apply, right?
> 
> Hmm.I suspect that bump-revision logic *should* apply when --switch is given
> with a *local* charm url *without* an explicit revision. Sane?

Done.

https://codereview.appspot.com/8540050/diff/1/cmd/juju/upgradecharm.go#newcod...
cmd/juju/upgradecharm.go:106: rev, err := repo.Latest(curl)
On 2013/04/25 15:43:30, fwereade wrote:
> On 2013/04/25 15:29:01, dimitern wrote:
> > See the question above.
> 
> So I think it's:
> 
> rev := curl.Revision
> if rev == -1 {
>     latest, err := repo.Latest(curl)
>     if err != nil {
>         return err
>     }
>     rev = latest
> }

Done.

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#n...
cmd/juju/upgradecharm_test.go:179: s.assertLocalRevision(c, 7, myriakPath)
On 2013/04/25 15:43:30, fwereade wrote:
> On 2013/04/25 15:29:01, dimitern wrote:
> > On 2013/04/25 15:09:06, fwereade wrote:
> > > On 2013/04/25 14:43:52, rog wrote:
> > > > also test switching from one cs: charm to another?
> > > > and with a revision number explicitly specified.
> > > 
> > > We have thus far gone with the shared agreement that if it works with one
> repo
> > > (local), we can expect it to work with the other. Hmm, can we now fake out
> the
> > > charm store? I think we can. If so, definite +1 on testing this in a wider
> > range
> > > of scenarios.
> > 
> > Sorry, I have no clue how to test this - ideas?
> 
> I'd be fine with setting charm.Store to some other repo and checking that it
> talks properly to what it *thinks* is the store

Not sure how to do that either - it seems I have to spin up a whole http server
for that. If that's what it takes, ok, but it seems an overkill.

[rog, 2013-04-26 15:06:32]

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#ne...
cmd/juju/upgradecharm.go:46: as the old charm.
probably worth reflowing all this text.

https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:51: to upgrade to, rather than the newest one. This
cannot be combined with --switch.
perhaps add:

To specify a given revision number with --switch, give
it in the charm URL, for instance cs:wordpress-5
would specify revision number 5 of the wordpress charm.

?

https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:123: latest, err := repo.Latest(curl)
i think this still isn't quite right.
if someone specifies a revision number in the charm url, this means we then
discard it and use the latest revision anyway.

i think fwereade's earlier suggestion is sound ("bump-revision logic should
apply when --switch is given
with a local charm url without an explicit revision")

AFAICS this code doesn't implement that logic.

https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:133: // This is not a local repository.
i think that's self evident from the condition on the if statement.

[dimitern, 2013-04-26 16:34:42]

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#ne...
cmd/juju/upgradecharm.go:46: as the old charm.
On 2013/04/26 15:06:33, rog wrote:
> probably worth reflowing all this text.

Done.

https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:51: to upgrade to, rather than the newest one. This
cannot be combined with --switch.
On 2013/04/26 15:06:33, rog wrote:
> perhaps add:
> 
> To specify a given revision number with --switch, give
> it in the charm URL, for instance cs:wordpress-5
> would specify revision number 5 of the wordpress charm.
> 
> ?

Done.

https://codereview.appspot.com/8540050/diff/10001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:133: // This is not a local repository.
On 2013/04/26 15:06:33, rog wrote:
> i think that's self evident from the condition on the if statement.

Removed.

[dimitern, 2013-04-26 16:37:19]

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#ne...
cmd/juju/upgradecharm.go:123: latest, err := repo.Latest(curl)
On 2013/04/26 15:06:33, rog wrote:
> i think this still isn't quite right.
> if someone specifies a revision number in the charm url, this means we then
> discard it and use the latest revision anyway.
> 
> i think fwereade's earlier suggestion is sound ("bump-revision logic should
> apply when --switch is given
> with a local charm url without an explicit revision")
> 
> AFAICS this code doesn't implement that logic.

Done.

[dimitern, 2013-04-26 17:50:33]

Please take a look.

[dimitern, 2013-04-26 17:57:11]

Please take a look.

[fwereade, 2013-04-28 10:41:25]

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#ol...
cmd/juju/upgradecharm.go:66: }
if c.SwitchURL != "" && c.Revision != -1 {
    return fmt.Errorf("--switch and --revision are mutually exclusive")
}

...or something

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ol...
cmd/juju/upgradecharm.go:95: return fmt.Errorf("already running latest charm
%q", curl)
Hmm. If the --force flag is set to something different to before, we might
actually want to allow this case (and other error below)?. Pre-existing bug,
doesn't have to be addressed in this CL, but if it isn't it should be recorded.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:111: return fmt.Errorf("cannot specify --switch and
--revision together")
This can and should be detected at Init() time.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:113: var err error
Not needed (but read the next comment before addressing it).

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:161: if considerBumpRevision {
I think this can all be simpler.

oldURL, _ := service.CharmURL()
var newURL *charm.URL
if c.SwitchURL != "" {
    // A new charm URL was explicitly specified.
    conf, err := conn.State.EnvironConfig()
    if err != nil {
        return err
    }
    newURL, err = charm.InferURL(c.SwitchURL, conf.DefaultSeries())
    if err != nil {
        return err
    }
} else {
    // No new URL was specified, but a revision might have been.
    newURL = oldURL.WithRevision(c.Revision)
}
repo, err := charm.InferRepository(newURL, ctx.AbsPath(c.RepoPath))
if err != nil {
    return err
}
// If no revision was explicitly specified by either Revision or
// SwitchURL, discover the latest from the repo.
if newURL.Revision == -1 {
    latest, err := repo.Latest(newURL)
    if err != nil {
        return err
    }
    newURL = newURL.WithRevision(latest)
}
if *newURL == *oldURL {
    if _, isLocal := repo.(*charm.LocalRepository); !isLocal {
        // etc

[dimitern, 2013-04-29 09:53:21]

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#ol...
cmd/juju/upgradecharm.go:66: }
On 2013/04/28 10:41:25, fwereade wrote:
> if c.SwitchURL != "" && c.Revision != -1 {
>     return fmt.Errorf("--switch and --revision are mutually exclusive")
> }
> 
> ...or something

Done.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ol...
cmd/juju/upgradecharm.go:95: return fmt.Errorf("already running latest charm
%q", curl)
On 2013/04/28 10:41:25, fwereade wrote:
> Hmm. If the --force flag is set to something different to before, we might
> actually want to allow this case (and other error below)?. Pre-existing bug,
> doesn't have to be addressed in this CL, but if it isn't it should be
recorded.

I added a TODO

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:111: return fmt.Errorf("cannot specify --switch and
--revision together")
On 2013/04/28 10:41:25, fwereade wrote:
> This can and should be detected at Init() time.

Done.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:113: var err error
On 2013/04/28 10:41:25, fwereade wrote:
> Not needed (but read the next comment before addressing it).
See the answer below.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:161: if considerBumpRevision {
On 2013/04/28 10:41:25, fwereade wrote:
> I think this can all be simpler.
> 
> oldURL, _ := service.CharmURL()
> var newURL *charm.URL
> if c.SwitchURL != "" {
>     // A new charm URL was explicitly specified.
>     conf, err := conn.State.EnvironConfig()
>     if err != nil {
>         return err
>     }
>     newURL, err = charm.InferURL(c.SwitchURL, conf.DefaultSeries())
>     if err != nil {
>         return err
>     }
> } else {
>     // No new URL was specified, but a revision might have been.
>     newURL = oldURL.WithRevision(c.Revision)
> }
> repo, err := charm.InferRepository(newURL, ctx.AbsPath(c.RepoPath))
> if err != nil {
>     return err
> }
> // If no revision was explicitly specified by either Revision or
> // SwitchURL, discover the latest from the repo.
> if newURL.Revision == -1 {
>     latest, err := repo.Latest(newURL)
>     if err != nil {
>         return err
>     }
>     newURL = newURL.WithRevision(latest)
> }
> if *newURL == *oldURL {
>     if _, isLocal := repo.(*charm.LocalRepository); !isLocal {
>         // etc

I prefer to keep it as is, if you don't mind, and I don't think it's any more
clear the suggested way.
Does it fulfill the required set of cases? I think so.

[fwereade, 2013-04-29 10:45:26]

I'm on the fence a little; let's have a chat about how bad the revision-bumping
situation is, it might be that the benefits of your implementation will win out.

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#ol...
cmd/juju/upgradecharm.go:95: return fmt.Errorf("already running latest charm
%q", curl)
On 2013/04/29 09:53:21, dimitern wrote:
> On 2013/04/28 10:41:25, fwereade wrote:
> > Hmm. If the --force flag is set to something different to before, we might
> > actually want to allow this case (and other error below)?. Pre-existing bug,
> > doesn't have to be addressed in this CL, but if it isn't it should be
> recorded.
> 
> I added a TODO

A bug would be great too please.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:27: originally deployed.
 An explicit revision can be chosen with the --revision flag.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:50: The new charm may add new relations and
configuration settings.
How about:

The --switch flag allows you to replace the charm with an entirely different
one. The new charm's URL and revision are inferred as they would be when running
a deploy command.

Please note that --switch is dangerous, because juju only has limited
information with which to determine compatibility; the operation will succeed,
regardless of potential havoc, so long as the following conditions hold:

- The new charm must declare all relations that the service is currently
participating in.
- All config settings shared by the old and new charms must have the same types.

--switch and --revision are mutually exclusive.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:56: specify revision number 5 of the wordpress charm.
Drop this para, --revision is best described at the top, I think; we just need
to mention it won't work with switch.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:76: f.StringVar(&c.SwitchURL, "switch", "", "charm URL
to upgrade to")
"crossgrade to a different charm" ?

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:77: f.IntVar(&c.Revision, "revision", -1, "revision
number to upgrade to")
"explicit revision of current charm" ?

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:161: if considerBumpRevision {
On 2013/04/29 09:53:21, dimitern wrote:
> On 2013/04/28 10:41:25, fwereade wrote:
> > I think this can all be simpler.
> > 
> > oldURL, _ := service.CharmURL()
> > var newURL *charm.URL
> > if c.SwitchURL != "" {
> >     // A new charm URL was explicitly specified.
> >     conf, err := conn.State.EnvironConfig()
> >     if err != nil {
> >         return err
> >     }
> >     newURL, err = charm.InferURL(c.SwitchURL, conf.DefaultSeries())
> >     if err != nil {
> >         return err
> >     }
> > } else {
> >     // No new URL was specified, but a revision might have been.
> >     newURL = oldURL.WithRevision(c.Revision)
> > }
> > repo, err := charm.InferRepository(newURL, ctx.AbsPath(c.RepoPath))
> > if err != nil {
> >     return err
> > }
> > // If no revision was explicitly specified by either Revision or
> > // SwitchURL, discover the latest from the repo.
> > if newURL.Revision == -1 {
> >     latest, err := repo.Latest(newURL)
> >     if err != nil {
> >         return err
> >     }
> >     newURL = newURL.WithRevision(latest)
> > }
> > if *newURL == *oldURL {
> >     if _, isLocal := repo.(*charm.LocalRepository); !isLocal {
> >         // etc
> 
> I prefer to keep it as is, if you don't mind, and I don't think it's any more
> clear the suggested way.

I find the current way rather unclear; it's a little tricky to keep track of
curl, scurl, bumpRevision, explicitRevision, rev, and considerBumpRevision;
especially considering that curl and scurl both have revisions embedded as well.
And I'm not sure that bumpRevision is even used any more...

If my suggestion is unclear, perhaps I should have commented it better; but I
think it's definitely easier to understand and keep track of oldURL and newURL
than all that other stuff... isn't it?

I'm not sure mine has ideal behaviour when --force and --revision interact with
local repos, but I don't think we have the *capacity* to do the Right Thing
there (ie upload a charm with a unique revision: but at the moment we're screwed
here in 2 or 3 different ways anyway, so it seems to be a moot point).

> Does it fulfill the required set of cases? I think so.

I'm not 100% sure, and that's the problem.

[dimitern, 2013-04-29 12:05:56]

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#ol...
cmd/juju/upgradecharm.go:95: return fmt.Errorf("already running latest charm
%q", curl)
On 2013/04/29 10:45:26, fwereade wrote:
> On 2013/04/29 09:53:21, dimitern wrote:
> > On 2013/04/28 10:41:25, fwereade wrote:
> > > Hmm. If the --force flag is set to something different to before, we might
> > > actually want to allow this case (and other error below)?. Pre-existing
bug,
> > > doesn't have to be addressed in this CL, but if it isn't it should be
> > recorded.
> > 
> > I added a TODO
> 
> A bug would be great too please.

Done.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:27: originally deployed.
On 2013/04/29 10:45:26, fwereade wrote:
>  An explicit revision can be chosen with the --revision flag.

Done.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:50: The new charm may add new relations and
configuration settings.
On 2013/04/29 10:45:26, fwereade wrote:
> How about:
> 
> The --switch flag allows you to replace the charm with an entirely different
> one. The new charm's URL and revision are inferred as they would be when
running
> a deploy command.
> 
> Please note that --switch is dangerous, because juju only has limited
> information with which to determine compatibility; the operation will succeed,
> regardless of potential havoc, so long as the following conditions hold:
> 
> - The new charm must declare all relations that the service is currently
> participating in.
> - All config settings shared by the old and new charms must have the same
types.
> 
> --switch and --revision are mutually exclusive.

Done.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:56: specify revision number 5 of the wordpress charm.
On 2013/04/29 10:45:26, fwereade wrote:
> Drop this para, --revision is best described at the top, I think; we just need
> to mention it won't work with switch.

I changed the wording to better describe both.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:76: f.StringVar(&c.SwitchURL, "switch", "", "charm URL
to upgrade to")
On 2013/04/29 10:45:26, fwereade wrote:
> "crossgrade to a different charm" ?

ewww.. ok :)

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:77: f.IntVar(&c.Revision, "revision", -1, "revision
number to upgrade to")
On 2013/04/29 10:45:26, fwereade wrote:
> "explicit revision of current charm" ?

Done.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:161: if considerBumpRevision {
On 2013/04/29 10:45:26, fwereade wrote:
> On 2013/04/29 09:53:21, dimitern wrote:
> > On 2013/04/28 10:41:25, fwereade wrote:
> > > I think this can all be simpler.
> > > 
> > > oldURL, _ := service.CharmURL()
> > > var newURL *charm.URL
> > > if c.SwitchURL != "" {
> > >     // A new charm URL was explicitly specified.
> > >     conf, err := conn.State.EnvironConfig()
> > >     if err != nil {
> > >         return err
> > >     }
> > >     newURL, err = charm.InferURL(c.SwitchURL, conf.DefaultSeries())
> > >     if err != nil {
> > >         return err
> > >     }
> > > } else {
> > >     // No new URL was specified, but a revision might have been.
> > >     newURL = oldURL.WithRevision(c.Revision)
> > > }
> > > repo, err := charm.InferRepository(newURL, ctx.AbsPath(c.RepoPath))
> > > if err != nil {
> > >     return err
> > > }
> > > // If no revision was explicitly specified by either Revision or
> > > // SwitchURL, discover the latest from the repo.
> > > if newURL.Revision == -1 {
> > >     latest, err := repo.Latest(newURL)
> > >     if err != nil {
> > >         return err
> > >     }
> > >     newURL = newURL.WithRevision(latest)
> > > }
> > > if *newURL == *oldURL {
> > >     if _, isLocal := repo.(*charm.LocalRepository); !isLocal {
> > >         // etc
> > 
> > I prefer to keep it as is, if you don't mind, and I don't think it's any
more
> > clear the suggested way.
> 
> I find the current way rather unclear; it's a little tricky to keep track of
> curl, scurl, bumpRevision, explicitRevision, rev, and considerBumpRevision;
> especially considering that curl and scurl both have revisions embedded as
well.
> And I'm not sure that bumpRevision is even used any more...
> 
> If my suggestion is unclear, perhaps I should have commented it better; but I
> think it's definitely easier to understand and keep track of oldURL and newURL
> than all that other stuff... isn't it?
> 
> I'm not sure mine has ideal behaviour when --force and --revision interact
with
> local repos, but I don't think we have the *capacity* to do the Right Thing
> there (ie upload a charm with a unique revision: but at the moment we're
screwed
> here in 2 or 3 different ways anyway, so it seems to be a moot point).
> 
> > Does it fulfill the required set of cases? I think so.
> 
> I'm not 100% sure, and that's the problem.

I changed it as suggested and added explicit revision handling, as discussed.

[fwereade, 2013-04-29 15:45:18]

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#ol...
cmd/juju/upgradecharm.go:100: } else if _, bumpRevision = ch.(*charm.Dir);
!bumpRevision {
...here -- in which case we should only set bumpRevision if explicitRevision is
false.

https://codereview.appspot.com/8540050/diff/32001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/8540050/diff/32001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:54: The new charm may add new relations and
configuration settings.
I think we can drop this line.

https://codereview.appspot.com/8540050/diff/32001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:145: if *newURL == *oldURL && !explicitRevision {
If newURL matches oldURL, we have to take this branch. We don't need to worry
about explicitRevision until...

https://codereview.appspot.com/8540050/diff/32001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:162: sch, err := conn.PutCharm(newURL, repo,
bumpRevision)
It's somewhat crazy that we can specify a URL with an additional param that
means "lol not really". But that's way out of scope.

[rog, 2013-04-29 16:11:50]

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 cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:76: f.StringVar(&c.SwitchURL, "switch", "", "charm URL
to upgrade to")
On 2013/04/29 12:05:56, dimitern wrote:
> On 2013/04/29 10:45:26, fwereade wrote:
> > "crossgrade to a different charm" ?
> 
> ewww.. ok :)

crossgrade??

how about just "change" ?

https://codereview.appspot.com/8540050/diff/32001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/8540050/diff/32001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:51: - All config settings shared by the old and new
charms must have the
that's not quite right, is it? i believe the new charm must declare all the
settings declared by the old, whereas this seems to imply that only the
intersection of old and new matters.

but maybe we the error message will be suitably self-explanatory if this
happens.

https://codereview.appspot.com/8540050/diff/32001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:145: if *newURL == *oldURL && !explicitRevision {
as discussed online, i'm not sure this is quite right yet.

[16:53:37] <rogpeppe> fwereade: but the case i'm thinking of is something like
this:
[16:54:14] <rogpeppe> fwereade: we have a local copy of a charm, deploy it, push
it to the charm store, switch to that, then make some changes to the local copy
and switch back to that
[16:54:48] <rogpeppe> fwereade: in that case, we want the version to be bumped,
but the urls are different

[dimitern, 2013-04-29 16:30:09]

Please take a look.

[fwereade, 2013-04-29 16:34:08]

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#ol...
cmd/juju/upgradecharm.go:100: } else if _, bumpRevision = ch.(*charm.Dir);
!bumpRevision {
We could just not bother checking this, and always setting bumpRevision in local
repos -- I don't think many people are putting bundles in local repos anyway,
and the error message from PutCharm should be clear enough to anyone who is.

https://codereview.appspot.com/8540050/diff/25002/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/8540050/diff/25002/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:153: // case (and the other error below). LP bug
#1174287
Move this comment up to the top case.

[dimitern, 2013-04-29 16:56:04]

*** 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