state/api/params: add CharmURL to ServiceInfo

state/api/params: add CharmURL to ServiceInfo

This is by way of an example of how new info
can be added to the data produced by the AllWatcher.

https://code.launchpad.net/~rogpeppe/juju-core/253-allwatcher-service-charm-info/+merge/154778

Requires: https://code.launchpad.net/~rogpeppe/juju-core/252-charm-url-json/+merge/154766

(do not edit description out of merge proposal)

[rog, 2013-03-21 18:10:49]

Please take a look.

[fwereade, 2013-03-22 00:59:02]

Let's not go too much further down this road without a discussion of where it's
going.

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

https://codereview.appspot.com/7950044/diff/2001/state/api/params/params.go#n...
state/api/params/params.go:237: // machineDoc etc).
Yeah, the eww continues. What's the plan for separating these? The direct
unmarshal is really pretty nasty...

[dave_cheney.net, 2013-03-22 01:04:33]

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

https://codereview.appspot.com/7950044/diff/2001/state/api/params/params.go#n...
state/api/params/params.go:237: // machineDoc etc).
This note sounds important, but it doesn't say how we should be careful around
them; just that we should be careful.

please add a second paragraph explaining how to properly care and feed for them.

https://codereview.appspot.com/7950044/diff/2001/state/api/params/params.go#n...
state/api/params/params.go:259: CharmURL *charm.URL
On line 282, we have CharmURL string. Is there a reason these differ ?

[gary.poster, 2013-03-22 09:23:26]

On 2013/03/22 00:59:02, fwereade wrote:
> Let's not go too much further down this road without a discussion of where
it's
> going.

FWIW, this particular branch is an example of more necessary work that the GUI
must have to declare initial success with Juju Core integration.  Maybe that was
already clear, but I wanted to highlight it.

Once we *do* have enough in the watcher code to declare initial GUI integration
success, I am sympathetic to the desire to keep additional development on the
megawatcher code as small as possible, until we've done some prep work for next
steps.

When we are ready to consider that prep work, I would like to first discuss more
broadly the priorities of goals for future GUI-related API changes. When we
consider optimization/scaling/maintainability of our watcher code within that
set, I'd like to have us sketch some success metrics, and analyze the current
solution in terms of those goals and sketched metrics, before prioritizing any
watcher refactoring work (including design).

[rog, 2013-03-22 17:04:42]

*** Submitted:

state/api/params: add CharmURL to ServiceInfo

This is by way of an example of how new info
can be added to the data produced by the AllWatcher.

R=fwereade, dfc, gary.poster
CC=
https://codereview.appspot.com/7950044

[fwereade, 2013-03-23 20:08:55]

The approach got my grudging approval in live discussion, which I'm ok with
considering an implicit LGTM, but it doesn't look like it got a second, and
dfc's comments were not addressed. This looks like enough of a beast that trying
to back it out from history will be hard, so I'm not going to insist, but please
follow up with dfc. He made a very good point re charm URLs -- I accept the
internal dirtiness, but I am very much unconvinced that it's smart to have
multiple representations of charm urls in the API.

FWIW, launchpad is confused about whether or not this was merged; please figure
out and fix whatever happened there too :).

[rog, 2013-03-24 12:11:27]

On 23 March 2013 20:08,  <fwereade@gmail.com> wrote:
> The approach got my grudging approval in live discussion, which I'm ok
> with considering an implicit LGTM, but it doesn't look like it got a
> second, and dfc's comments were not addressed. This looks like enough of
> a beast that trying to back it out from history will be hard, so I'm not
> going to insist, but please follow up with dfc. He made a very good
> point re charm URLs -- I accept the internal dirtiness, but I am very
> much unconvinced that it's smart to have multiple representations of
> charm urls in the API.

Oops, for some reason I had got it in my head that all my pending
CLs were approved without qualification, and submitted them all in
quick succession without re-checking each one. *smacks palm with a ruler*

I will update to address earlier comments.

> FWIW, launchpad is confused about whether or not this was merged; please
> figure out and fix whatever happened there too :).

Looking at trunk, it's definitely merged. Will check lp on Monday.

  cheers,
    rog.