Add upgrade-charm to the API

Add upgrade-charm to the API

Per #1100904, we need to be able to upgrade a charm from the API for the GUI.  This adds the upgradecharm command to the statecmds.

https://code.launchpad.net/~makyo/juju-core/upgrade-charm-api-1171548/+merge/160469

(do not edit description out of merge proposal)

[matthew.scott, 2013-04-23 18:35:06]

Please take a look.

[dimitern, 2013-04-24 08:11:17]

Looks good, just some suggestions.

https://codereview.appspot.com/8926043/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/8926043/diff/1/state/api/apiclient.go#newcode88
state/api/apiclient.go:88: // ServiceUpgradeCharm upgrades the service's charm
to the current version.
s/current/latest available/ version in the repository ?

https://codereview.appspot.com/8926043/diff/1/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/8926043/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:305: // ServiceUpgradeCharm upgrades the service's
charm to the current version.
ditto

https://codereview.appspot.com/8926043/diff/1/state/statecmd/upgradecharm_tes...
File state/statecmd/upgradecharm_test.go (right):

https://codereview.appspot.com/8926043/diff/1/state/statecmd/upgradecharm_tes...
state/statecmd/upgradecharm_test.go:23: err     string
please, add some tests with error checking or remove err altogether.

https://codereview.appspot.com/8926043/diff/1/state/statecmd/upgradecharm_tes...
state/statecmd/upgradecharm_test.go:47: c.Logf("test %d. %s", i, t.about)
move this as a first log line

[bac, 2013-04-24 14:08:29]

LGTM with Dimitern's changes.

https://codereview.appspot.com/8926043/diff/1/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/8926043/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:437: // signatures match.
Why did you add this comment here and not in your new function?

[matthew.scott, 2013-04-24 15:18:53]

Thanks for the reviews.

https://codereview.appspot.com/8926043/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/8926043/diff/1/state/api/apiclient.go#newcode88
state/api/apiclient.go:88: // ServiceUpgradeCharm upgrades the service's charm
to the current version.
On 2013/04/24 08:11:17, dimitern wrote:
> s/current/latest available/ version in the repository ?

Done.

https://codereview.appspot.com/8926043/diff/1/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/8926043/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:437: // signatures match.
On 2013/04/24 14:08:29, bac wrote:
> Why did you add this comment here and not in your new function?

Because I missed :P  It fits, though, so I'll leave it as documentation.

https://codereview.appspot.com/8926043/diff/1/state/apiserver/apiserver.go
File state/apiserver/apiserver.go (right):

https://codereview.appspot.com/8926043/diff/1/state/apiserver/apiserver.go#ne...
state/apiserver/apiserver.go:305: // ServiceUpgradeCharm upgrades the service's
charm to the current version.
On 2013/04/24 08:11:17, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/8926043/diff/1/state/statecmd/upgradecharm_tes...
File state/statecmd/upgradecharm_test.go (right):

https://codereview.appspot.com/8926043/diff/1/state/statecmd/upgradecharm_tes...
state/statecmd/upgradecharm_test.go:23: err     string
On 2013/04/24 08:11:17, dimitern wrote:
> please, add some tests with error checking or remove err altogether.

Removed; leftover from stubbing out based on existing tests.  There are many
existing tests in cmd/juju/upgradecharm_test.go and I didn't want to duplicate
code.

https://codereview.appspot.com/8926043/diff/1/state/statecmd/upgradecharm_tes...
state/statecmd/upgradecharm_test.go:47: c.Logf("test %d. %s", i, t.about)
On 2013/04/24 08:11:17, dimitern wrote:
> move this as a first log line

Done.

[dimitern, 2013-04-24 15:51:46]

Thanks, LGTM.

[fwereade, 2013-04-24 16:24:39]

Tests need to move to the right place as well, not just the code ;).

https://codereview.appspot.com/8926043/diff/1/state/statecmd/upgradecharm_tes...
File state/statecmd/upgradecharm_test.go (right):

https://codereview.appspot.com/8926043/diff/1/state/statecmd/upgradecharm_tes...
state/statecmd/upgradecharm_test.go:23: err     string
On 2013/04/24 15:18:53, matthew.scott wrote:
> On 2013/04/24 08:11:17, dimitern wrote:
> > please, add some tests with error checking or remove err altogether.
> 
> Removed; leftover from stubbing out based on existing tests.  There are many
> existing tests in cmd/juju/upgradecharm_test.go and I didn't want to duplicate
> code.

This is where we should be having the detailed tests -- right next to the logic.
Please move the appropriate ones from the command into this package; just leave
(1) the tests that exercise the arg-parsing and (2) one or two tracer bullets to
demonstrate success/failure against real state.

[matthew.scott, 2013-04-24 16:46:31]

*** Submitted:

Add upgrade-charm to the API

Per #1100904, we need to be able to upgrade a charm from the API for the GUI. 
This adds the upgradecharm command to the statecmds.

R=dimitern, bac, fwereade
CC=
https://codereview.appspot.com/8926043

[rog, 2013-04-24 16:51:11]

This is going in a good direction, but I think there's some more work to be done
with respect to local charms. I've made some comments below; until that issue is
addressed, this does not LGTM.

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

https://codereview.appspot.com/8926043/diff/1/cmd/juju/upgradecharm.go#newcode87
cmd/juju/upgradecharm.go:87: err = statecmd.ServiceUpgradeCharm(conn.State,
params)
s/err = /return/
?

https://codereview.appspot.com/8926043/diff/1/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/8926043/diff/1/state/api/params/params.go#newc...
state/api/params/params.go:81: RepoPath    string // defaults to JUJU_REPOSITORY
i'm not sure that defaulting to JUJU_REPOSITORY is correct here - the API server
won't have a sensible JUJU_REPOSITORY set, will it?

https://codereview.appspot.com/8926043/diff/1/state/apiserver/api_test.go
File state/apiserver/api_test.go (right):

https://codereview.appspot.com/8926043/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:428: err :=
st.Client().ServiceUpgradeCharm("no-such", false, "")
i think we're missing a test - the op* functions are to test permissions only.
i'd like to a see an independent test that tests the ServiceUpgradeCharm
functionality specifically.
given that there are already tests in statecmd, just something minimal will do
until the code and tests moves from statecmd to apiserver.

https://codereview.appspot.com/8926043/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:436: // This test only checks that the call is made
without error, ensuring the
s/ensuring the signatures match/ensuring the correct permission check is made/
?

https://codereview.appspot.com/8926043/diff/1/state/apiserver/api_test.go#new...
state/apiserver/api_test.go:437: // signatures match.
On 2013/04/24 14:08:29, bac wrote:
> Why did you add this comment here and not in your new function?

i'd be happy if the comment is added to all similar places.

https://codereview.appspot.com/8926043/diff/1/state/statecmd/upgradecharm.go
File state/statecmd/upgradecharm.go (right):

https://codereview.appspot.com/8926043/diff/1/state/statecmd/upgradecharm.go#...
state/statecmd/upgradecharm.go:15: conn, err := juju.NewConnFromState(state)
better would be to have the Conn as an argument - cmd/juju already has one
around.

https://codereview.appspot.com/8926043/diff/1/state/statecmd/upgradecharm.go#...
state/statecmd/upgradecharm.go:24: repo, err := charm.InferRepository(curl,
args.RepoPath)
i'm not sure that this logic is right when running in the API server, where we
never want to allow local charms AFAICS.

i wonder if the charm repository should be an extra argument to
ServiceUpgradeCharm; then the API can always pass charm.Store, and the cmd/juju
code can call InferRepository.

the api entry point should check for and reject charm urls with a local schema
(in the future it will probably use a separate entry point for uploading
charms).

I'd take all local-specific stuff out of this function and leave it in cmd/juju.