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

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

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