I'm on the fence a little; let's have a chat about how bad the
revision-bumping situation is, it might be that the benefits of your
implementation will win out.


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#oldcode95
cmd/juju/upgradecharm.go:95: return fmt.Errorf("already running latest
charm %q", curl)
On 2013/04/29 09:53:21, dimitern wrote:
> 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

A bug would be great too please.

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#newcode27
cmd/juju/upgradecharm.go:27: originally deployed.
  An explicit revision can be chosen with the --revision flag.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode50
cmd/juju/upgradecharm.go:50: The new charm may add new relations and
configuration settings.
How about:

The --switch flag allows you to replace the charm with an entirely
different one. The new charm's URL and revision are inferred as they
would be when running a deploy command.

Please note that --switch is dangerous, because juju only has limited
information with which to determine compatibility; the operation will
succeed, regardless of potential havoc, so long as the following
conditions hold:

- The new charm must declare all relations that the service is currently
participating in.
- All config settings shared by the old and new charms must have the
same types.

--switch and --revision are mutually exclusive.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode56
cmd/juju/upgradecharm.go:56: specify revision number 5 of the wordpress
charm.
Drop this para, --revision is best described at the top, I think; we
just need to mention it won't work with switch.

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode76
cmd/juju/upgradecharm.go:76: f.StringVar(&c.SwitchURL, "switch", "",
"charm URL to upgrade to")
"crossgrade to a different charm" ?

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode77
cmd/juju/upgradecharm.go:77: f.IntVar(&c.Revision, "revision", -1,
"revision number to upgrade to")
"explicit revision of current charm" ?

https://codereview.appspot.com/8540050/diff/23001/cmd/juju/upgradecharm.go#newcode161
cmd/juju/upgradecharm.go:161: if considerBumpRevision {
On 2013/04/29 09:53:21, dimitern wrote:
> 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.

I find the current way rather unclear; it's a little tricky to keep
track of curl, scurl, bumpRevision, explicitRevision, rev, and
considerBumpRevision; especially considering that curl and scurl both
have revisions embedded as well. And I'm not sure that bumpRevision is
even used any more...

If my suggestion is unclear, perhaps I should have commented it better;
but I think it's definitely easier to understand and keep track of
oldURL and newURL than all that other stuff... isn't it?

I'm not sure mine has ideal behaviour when --force and --revision
interact with local repos, but I don't think we have the *capacity* to
do the Right Thing there (ie upload a charm with a unique revision: but
at the moment we're screwed here in 2 or 3 different ways anyway, so it
seems to be a moot point).

> Does it fulfill the required set of cases? I think so.

I'm not 100% sure, and that's the problem.

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