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

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