Code review comment for lp://staging/~dimitern/juju-core/038-upgrade-charm-switch

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#newcode100
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#newcode106
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#newcode179
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

https://codereview.appspot.com/8540050/