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

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 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#oldcode95
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#newcode111
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#newcode113
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#newcode161
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.

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