cmd: upgrade-charm basic implementation

cmd: upgrade-charm basic implementation

This is the basic upgrade-charm command, no
flags yet implemented, only a --repository
argument.

https://code.launchpad.net/~dimitern/juju-core/016-upgrade-charm-cmd/+merge/154411

(do not edit description out of merge proposal)

[fwereade, 2013-03-20 17:14:46]

Bit confused by the tests, and pretty sure the implementation needs some work.

https://codereview.appspot.com/7927043/diff/1/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/7927043/diff/1/cmd/juju/upgradecharm.go#newcode59
cmd/juju/upgradecharm.go:59: curl, err := charm.InferURL(c.ServiceName,
conf.DefaultSeries())
I don't think this is right. We should use the service's current charm URL to
infer a repository, then ask that repo for the latest version of the same charm,
and abort unless it's newer than the deployed one (modulo BumpRevision in local
repos, like in deploy).

https://codereview.appspot.com/7927043/diff/1/cmd/juju/upgradecharm.go#newcode67
cmd/juju/upgradecharm.go:67: // TODO(dimitern): this will change once we add the
--switch flag.
I think it needs to change now -- surely you still need to InferRepository, just
as we do in deploy, rather than always assuming the charm store?

https://codereview.appspot.com/7927043/diff/1/cmd/juju/upgradecharm_test.go
File cmd/juju/upgradecharm_test.go (right):

https://codereview.appspot.com/7927043/diff/1/cmd/juju/upgradecharm_test.go#n...
cmd/juju/upgradecharm_test.go:45: err = runUpgradeCharm(c, "riak")
I'm baffled as to where rev8 comes from here.

[dimitern, 2013-03-25 16:43:44]

Please take a look.

https://codereview.appspot.com/7927043/diff/1/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/7927043/diff/1/cmd/juju/upgradecharm.go#newcode59
cmd/juju/upgradecharm.go:59: curl, err := charm.InferURL(c.ServiceName,
conf.DefaultSeries())
On 2013/03/20 17:14:46, fwereade wrote:
> I don't think this is right. We should use the service's current charm URL to
> infer a repository, then ask that repo for the latest version of the same
charm,
> and abort unless it's newer than the deployed one (modulo BumpRevision in
local
> repos, like in deploy).

Done.

https://codereview.appspot.com/7927043/diff/1/cmd/juju/upgradecharm.go#newcode67
cmd/juju/upgradecharm.go:67: // TODO(dimitern): this will change once we add the
--switch flag.
On 2013/03/20 17:14:46, fwereade wrote:
> I think it needs to change now -- surely you still need to InferRepository,
just
> as we do in deploy, rather than always assuming the charm store?

Done.

https://codereview.appspot.com/7927043/diff/1/cmd/juju/upgradecharm_test.go
File cmd/juju/upgradecharm_test.go (right):

https://codereview.appspot.com/7927043/diff/1/cmd/juju/upgradecharm_test.go#n...
cmd/juju/upgradecharm_test.go:45: err = runUpgradeCharm(c, "riak")
On 2013/03/20 17:14:46, fwereade wrote:
> I'm baffled as to where rev8 comes from here.

riak charm's revision is 7.

[fwereade, 2013-03-25 17:45:43]

Looking good, but I've had a few more thoughts since a couple of hours ago ;p

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:18: RepoPath    string // defaults to JUJU_REPOSITORY
FWIW the description is inaccurate, we haven't yet done --revision.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:22: <service> needs to be an existing deployed service,
whose charm you want to upgrade.
I'm not sure this adds much... but it would be good to cover to what version,
and from what repository, the charm will be upgraded.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:80: }
I know what we should do here. Once we know it *is* a local repo, it's
relatively fast to get the charm; if we do that, we can see whether it's a dir
or a bundle directly and pick bumpRevision based on that.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:89: return fmt.Errorf("already running latest charm
%q", curl)
If we do the above, we can drop this bit -- and also revert the
ErrCannotBumpRevision change (but keep the added test).

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm_test.go
File cmd/juju/upgradecharm_test.go (right):

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm_test.g...
cmd/juju/upgradecharm_test.go:118: }
Just a thought -- we don't have tests for --repository. Checking that an empty
repo causes a failure ir probably good enough.

https://codereview.appspot.com/7927043/diff/5001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/7927043/diff/5001/juju/conn.go#newcode169
juju/conn.go:169: return nil, ErrCannotBumpRevision
TBH, I think the old error was better. Sorry about that.

[rog, 2013-03-25 17:54:24]

looks really good in general. a few thoughts below.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/deploy_test.go
File cmd/juju/deploy_test.go (right):

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/deploy_test.go#newc...
cmd/juju/deploy_test.go:171: c.Assert(err, ErrorMatches, "cannot increment
revision of non-directory charm")
+1 to change to use "revision", but -1 to loss of contextual information in the
error message.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:22: <service> needs to be an existing deployed service,
whose charm you want to upgrade.
perhaps the docs could mention that the repository defaults to $JUJ_REPOSITORY?

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:57: func (c *UpgradeCharmCommand) Run(ctx *cmd.Context)
error {
i wonder if, to save the gui guys the extra effort, the logic in this function
could go straight into statecmd.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:67: curl, _ := service.CharmURL()
it seems a bit weird that CharmURL returns the "should upgrade" status too. not
your issue, of course, just saying.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:68: repo, err := charm.InferRepository(curl,
ctx.AbsPath(c.RepoPath))
is there a reason the path needs to be absolute?

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:89: return fmt.Errorf("already running latest charm
%q", curl)
tbh, i think the ErrCannotBumpRevision error might be more self-explanatory
here.

[dimitern, 2013-03-25 19:18:50]

Please take a look.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/deploy_test.go
File cmd/juju/deploy_test.go (right):

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/deploy_test.go#newc...
cmd/juju/deploy_test.go:171: c.Assert(err, ErrorMatches, "cannot increment
revision of non-directory charm")
On 2013/03/25 17:54:24, rog wrote:
> +1 to change to use "revision", but -1 to loss of contextual information in
the
> error message.

Reverted to the old message, but replaced "version" with "revision".

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:18: RepoPath    string // defaults to JUJU_REPOSITORY
On 2013/03/25 17:45:43, fwereade wrote:
> FWIW the description is inaccurate, we haven't yet done --revision.

Done.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:22: <service> needs to be an existing deployed service,
whose charm you want to upgrade.
On 2013/03/25 17:54:24, rog wrote:
> perhaps the docs could mention that the repository defaults to
$JUJ_REPOSITORY?

Done.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:22: <service> needs to be an existing deployed service,
whose charm you want to upgrade.
On 2013/03/25 17:45:43, fwereade wrote:
> I'm not sure this adds much... but it would be good to cover to what version,
> and from what repository, the charm will be upgraded.

Done, although not sure if my wording is correct, please check.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:57: func (c *UpgradeCharmCommand) Run(ctx *cmd.Context)
error {
On 2013/03/25 17:54:24, rog wrote:
> i wonder if, to save the gui guys the extra effort, the logic in this function
> could go straight into statecmd.

I prefer not to do this now. Anyway it should be trivial for them to do it
later, the command is not much different from what they already ported to
statecmd.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:68: repo, err := charm.InferRepository(curl,
ctx.AbsPath(c.RepoPath))
On 2013/03/25 17:54:24, rog wrote:
> is there a reason the path needs to be absolute?

Not sure; deploy does it like this, so I just used the same behavior here.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:80: }
On 2013/03/25 17:45:43, fwereade wrote:
> I know what we should do here. Once we know it *is* a local repo, it's
> relatively fast to get the charm; if we do that, we can see whether it's a dir
> or a bundle directly and pick bumpRevision based on that.

Done.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:89: return fmt.Errorf("already running latest charm
%q", curl)
On 2013/03/25 17:45:43, fwereade wrote:
> If we do the above, we can drop this bit -- and also revert the
> ErrCannotBumpRevision change (but keep the added test).

Done.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:89: return fmt.Errorf("already running latest charm
%q", curl)
On 2013/03/25 17:54:24, rog wrote:
> tbh, i think the ErrCannotBumpRevision error might be more self-explanatory
> here.

I think the code now is actually simpler to read.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm_test.go
File cmd/juju/upgradecharm_test.go (right):

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm_test.g...
cmd/juju/upgradecharm_test.go:118: }
On 2013/03/25 17:45:43, fwereade wrote:
> Just a thought -- we don't have tests for --repository. Checking that an empty
> repo causes a failure ir probably good enough.

Done.
Although I should argue none of the other commands have such tests.

https://codereview.appspot.com/7927043/diff/5001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/7927043/diff/5001/juju/conn.go#newcode169
juju/conn.go:169: return nil, ErrCannotBumpRevision
On 2013/03/25 17:45:43, fwereade wrote:
> TBH, I think the old error was better. Sorry about that.

Reverted to the old error, but replaced "version" with "revision".

[dimitern, 2013-03-25 19:22:29]

Actually, wrt the --repository test because repoSuite *always* overwrites
JUJU_REPOSITORY in SetUpSuite it's not possible to check for failures when the
argument is empty.

[fwereade, 2013-03-26 08:38:52]

Basically LGTM, but it *would* be really nice to add a trivial passthrough for
SetCharm in the API as rog suggested.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:67: curl, _ := service.CharmURL()
On 2013/03/25 17:54:24, rog wrote:
> it seems a bit weird that CharmURL returns the "should upgrade" status too.
not
> your issue, of course, just saying.

It might be clearest if we just dropped the existing Charm method and renamed
CharmURL methods to Charm across the board. But, yeah, out of scope, just
ruminating.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:68: repo, err := charm.InferRepository(curl,
ctx.AbsPath(c.RepoPath))
On 2013/03/25 19:18:50, dimitern wrote:
> On 2013/03/25 17:54:24, rog wrote:
> > is there a reason the path needs to be absolute?
> 
> Not sure; deploy does it like this, so I just used the same behavior here.

Yes: the working directory of the command is not necessarily the working
directory of the process.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:89: return fmt.Errorf("already running latest charm
%q", curl)
On 2013/03/25 17:54:24, rog wrote:
> tbh, i think the ErrCannotBumpRevision error might be more self-explanatory
> here.

The only reason we might have got it is because we were guessing whether or not
it was incrementable. Now we're figuring it out for sure we can be much clearer.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm_test.go
File cmd/juju/upgradecharm_test.go (right):

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm_test.g...
cmd/juju/upgradecharm_test.go:118: }
On 2013/03/25 19:18:50, dimitern wrote:
> On 2013/03/25 17:45:43, fwereade wrote:
> > Just a thought -- we don't have tests for --repository. Checking that an
empty
> > repo causes a failure ir probably good enough.
> 
> Done.
> Although I should argue none of the other commands have such tests.

And I should argue that, if they don't, such tests should probably be added ;p.
See above, in the next changeset; it should be easy to verify in both cases.

https://codereview.appspot.com/7927043/diff/5001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/7927043/diff/5001/juju/conn.go#newcode169
juju/conn.go:169: return nil, ErrCannotBumpRevision
On 2013/03/25 19:18:50, dimitern wrote:
> On 2013/03/25 17:45:43, fwereade wrote:
> > TBH, I think the old error was better. Sorry about that.
> 
> Reverted to the old error, but replaced "version" with "revision".

Perfect, thanks.

https://codereview.appspot.com/7927043/diff/15001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/7927043/diff/15001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:27: in the charm store or from a specified local
repository.
How about (for now) something like:

When no flags are set, the service's charm will be upgraded to the latest
revision available in the repository from which it was originally deployed.

If the charm came from a local repository, its path will be assumed to be
$JUJU_REPOSITORY unless overridden by --repository. If there is no newer
revision of a local charm directory, the local directory's revision will be
automatically incremented to create a newer version.

The local repository behaviour is tuned specifically to the workflow of a charm
author working on a single client machine; use of local repositories from
multiple clients is not supported and may lead to confusing behaviour.

https://codereview.appspot.com/7927043/diff/15001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:86: if _, bumpRevision = repo.(*charm.LocalRepository);
!bumpRevision {
I think it's confusing to set bumpRevision here; it'll always either induce an
error or be overwritten again later. Calling it isLocal (in this line only)
should make it clearer.

https://codereview.appspot.com/7927043/diff/15001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:89: // This is a local repository.
Drop the else and save the indentation.

https://codereview.appspot.com/7927043/diff/15001/cmd/juju/upgradecharm_test.go
File cmd/juju/upgradecharm_test.go (right):

https://codereview.appspot.com/7927043/diff/15001/cmd/juju/upgradecharm_test....
cmd/juju/upgradecharm_test.go:36: }
I think it's probably a good idea to add a test that explicitly sets
$JUJU_REPOSITORY for the same purpose. IIRC repoSuite will reset that var at the
end of the test anyway (right?).

[rog, 2013-03-26 11:20:06]

LGTM with william's changes applied.

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/7927043/diff/5001/cmd/juju/upgradecharm.go#new...
cmd/juju/upgradecharm.go:89: return fmt.Errorf("already running latest charm
%q", curl)
On 2013/03/26 08:38:52, fwereade wrote:
> On 2013/03/25 17:54:24, rog wrote:
> > tbh, i think the ErrCannotBumpRevision error might be more self-explanatory
> > here.
> 
> The only reason we might have got it is because we were guessing whether or
not
> it was incrementable. Now we're figuring it out for sure we can be much
clearer.

i wasn't suggesting the change to make the code simpler to read, but because
from a user point of view, if i try to deploy a local bundled charm, i think
that "already running latest charm" isn't so useful an error as "cannot change
revision on charm because it's not a directory".

https://codereview.appspot.com/7927043/diff/15001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/7927043/diff/15001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:86: if _, bumpRevision = repo.(*charm.LocalRepository);
!bumpRevision {
On 2013/03/26 08:38:52, fwereade wrote:
> I think it's confusing to set bumpRevision here; it'll always either induce an
> error or be overwritten again later. Calling it isLocal (in this line only)
> should make it clearer.

good catch. +1.

https://codereview.appspot.com/7927043/diff/15001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:89: // This is a local repository.
On 2013/03/26 08:38:52, fwereade wrote:
> Drop the else and save the indentation.

+1

[dimitern, 2013-03-26 12:24:51]

Please take a look.

https://codereview.appspot.com/7927043/diff/15001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/7927043/diff/15001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:27: in the charm store or from a specified local
repository.
On 2013/03/26 08:38:52, fwereade wrote:
> How about (for now) something like:
> 
> When no flags are set, the service's charm will be upgraded to the latest
> revision available in the repository from which it was originally deployed.
> 
> If the charm came from a local repository, its path will be assumed to be
> $JUJU_REPOSITORY unless overridden by --repository. If there is no newer
> revision of a local charm directory, the local directory's revision will be
> automatically incremented to create a newer version.
> 
> The local repository behaviour is tuned specifically to the workflow of a
charm
> author working on a single client machine; use of local repositories from
> multiple clients is not supported and may lead to confusing behaviour.

Done.

https://codereview.appspot.com/7927043/diff/15001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:86: if _, bumpRevision = repo.(*charm.LocalRepository);
!bumpRevision {
On 2013/03/26 08:38:52, fwereade wrote:
> I think it's confusing to set bumpRevision here; it'll always either induce an
> error or be overwritten again later. Calling it isLocal (in this line only)
> should make it clearer.
I was thinking the same yesterday :) Good point, I'm changing it.

https://codereview.appspot.com/7927043/diff/15001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:89: // This is a local repository.
On 2013/03/26 08:38:52, fwereade wrote:
> Drop the else and save the indentation.

Done.

https://codereview.appspot.com/7927043/diff/15001/cmd/juju/upgradecharm_test.go
File cmd/juju/upgradecharm_test.go (right):

https://codereview.appspot.com/7927043/diff/15001/cmd/juju/upgradecharm_test....
cmd/juju/upgradecharm_test.go:36: }
On 2013/03/26 08:38:52, fwereade wrote:
> I think it's probably a good idea to add a test that explicitly sets
> $JUJU_REPOSITORY for the same purpose. IIRC repoSuite will reset that var at
the
> end of the test anyway (right?).

Done.

[fwereade, 2013-03-26 13:01:08]

LGTM, thanks

https://codereview.appspot.com/7927043/diff/28001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/7927043/diff/28001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:28: automatically incremented to create a newer
version.
s/version/charm/ perhaps?

https://codereview.appspot.com/7927043/diff/28001/cmd/juju/upgradecharm_test.go
File cmd/juju/upgradecharm_test.go (right):

https://codereview.appspot.com/7927043/diff/28001/cmd/juju/upgradecharm_test....
cmd/juju/upgradecharm_test.go:44: err = runUpgradeCharm(c, "riak",
"--repository=")
Maybe just drop the "--repository=" arg here?

[dimitern, 2013-03-26 13:05:44]

*** Submitted:

cmd: upgrade-charm basic implementation

This is the basic upgrade-charm command, no
flags yet implemented, only a --repository
argument.

R=fwereade, rog
CC=
https://codereview.appspot.com/7927043

https://codereview.appspot.com/7927043/diff/28001/cmd/juju/upgradecharm.go
File cmd/juju/upgradecharm.go (right):

https://codereview.appspot.com/7927043/diff/28001/cmd/juju/upgradecharm.go#ne...
cmd/juju/upgradecharm.go:28: automatically incremented to create a newer
version.
On 2013/03/26 13:01:08, fwereade wrote:
> s/version/charm/ perhaps?

Done.

https://codereview.appspot.com/7927043/diff/28001/cmd/juju/upgradecharm_test.go
File cmd/juju/upgradecharm_test.go (right):

https://codereview.appspot.com/7927043/diff/28001/cmd/juju/upgradecharm_test....
cmd/juju/upgradecharm_test.go:44: err = runUpgradeCharm(c, "riak",
"--repository=")
On 2013/03/26 13:01:08, fwereade wrote:
> Maybe just drop the "--repository=" arg here?

I prefer to leave it as is; it reads better I think.