Code review comment for lp://staging/~makyo/juju-core/upgradecharm3-1171548

Revision history for this message
William Reade (fwereade) wrote :

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#newcode134
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#newcode16
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?).

https://codereview.appspot.com/9975045/

« Back to merge proposal