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

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

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