Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(5602)

Issue 9975045: Add UpgradeCharm to the API

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 11 months ago by matthew.scott
Modified:
10 years, 11 months ago
Reviewers:
mp+167331, fwereade
Visibility:
Public.

Description

Add UpgradeCharm to the API This branch adds upgrade charm functionality to the API (branch 3/3 in the process). https://code.launchpad.net/~makyo/juju-core/upgradecharm3-1171548/+merge/167331 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+192 lines, -0 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M state/api/client.go View 1 chunk +10 lines, -0 lines 0 comments Download
M state/api/params/params.go View 1 chunk +7 lines, -0 lines 0 comments Download
M state/apiserver/client.go View 1 chunk +23 lines, -0 lines 1 comment Download
M state/apiserver/perm_test.go View 2 chunks +12 lines, -0 lines 0 comments Download
A state/statecmd/upgradecharm.go View 1 chunk +21 lines, -0 lines 1 comment Download
A state/statecmd/upgradecharm_test.go View 1 chunk +117 lines, -0 lines 0 comments Download

Messages

Total messages: 2
matthew.scott
Please take a look.
10 years, 11 months ago (2013-06-04 16:09:14 UTC) #1
fwereade
10 years, 11 months ago (2013-06-05 10:51:23 UTC) #2
NOT LGTM until we've spoken -- but I might be convinced to go ahead with this on
the condition of followups we agree on live.

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

https://codereview.appspot.com/9975045/diff/1/state/apiserver/client.go#newco...
state/apiserver/client.go:134: // Set bumpRevision to false when working with
the CharmStore.
We cannot help but work with the charm store here. Looking back in time a bit,
it seems that we failed in reviewing ServiceDeploy: no blame attaches for
continuing along the "approved" path, ofc, but we can't go on this way.

The issue is simply that there is no local repo on the API server. The local
repo is in practice a pretty broken concept regardless, but that's by the by;
the fundamental problem is that the client API for deploying and upgrading
charms does need to be able to use charms from local repos. The only way to do
this, AFAICT, is to implement a PutCharm(?) API that causes a bundled charm on
the client to be stored in the environment and in state, and then to explicitly
reference that charm in subsequent deploy and upgrade ops.

I thought I'd kinda handed the details of this problem over to rogpeppe; rog, do
you have anything to add to this? What's the plan with charm store charms here?
It looks as though we're not getting necessarily using revisioned charm URLs...
not sure how this is sane.

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

https://codereview.appspot.com/9975045/diff/1/state/statecmd/upgradecharm.go#...
state/statecmd/upgradecharm.go:16: sch, err := conn.PutCharm(curl, repo,
bumpRevision)
I'm not sure this should happen on the server side at all; the CLI and the GUI
are each separately in a position to determine, and upload, *exactly* the
desired charm. The bumpRevision logic only makes sense in the context of a local
repo, and AFAIK nobody has yet implemented the AddCharm(?) API method that's a
prerequisite for use of local charms over the API.

I can see the use case for something *like* this PutCharm method, that just
copies direct from the charm store into environment storage, but that's more an
AddCharm: PutCharm STM like it implies actually uploading some bytes from the
other end of the connection (and that's what it actually does... right?).
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b