state: service config settings in AllWatcher

state: service config settings in AllWatcher

This does not provide default settings, so the
client needs to be intelligent about interpreting
the information, but that is possibly a reasonable
thing (saves network bandwidth - the client
has access to the charm metadata information).

https://code.launchpad.net/~rogpeppe/juju-core/286-allwatcher-service-config/+merge/159037

(do not edit description out of merge proposal)

[rog, 2013-04-15 22:46:13]

Please take a look.

[thumper, 2013-04-16 00:01:42]

https://codereview.appspot.com/8761045/diff/2001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/8761045/diff/2001/state/megawatcher.go#newcode306
state/megawatcher.go:306: info0 := store.Get(parentId)
Sorry, but info0 is a horrible name.  What is it actually representing?

https://codereview.appspot.com/8761045/diff/2001/state/megawatcher.go#newcode335
state/megawatcher.go:335: func (a *backingSettings) mongoId() interface{} {
Why is this not (s *backingSettings) like the others?

[rog, 2013-04-16 08:10:44]

https://codereview.appspot.com/8761045/diff/2001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/8761045/diff/2001/state/megawatcher.go#newcode306
state/megawatcher.go:306: info0 := store.Get(parentId)
On 2013/04/16 00:01:42, thumper wrote:
> Sorry, but info0 is a horrible name.  What is it actually representing?

you're probably right, but i'm not sure of a better one.
it's the EntityInfo object that we've fetched from the store and that we update
afterwards.

it's really just a differently typed version of the "info" in the type switch.
perhaps if i kept this named as "info" and named the variable in the type switch
to "info1" it might be more obvious.

i think it's good that the names of the two variables are very closely related
because they both point to and signify exactly the same thing.

suggestions?

https://codereview.appspot.com/8761045/diff/2001/state/megawatcher.go#newcode335
state/megawatcher.go:335: func (a *backingSettings) mongoId() interface{} {
On 2013/04/16 00:01:42, thumper wrote:
> Why is this not (s *backingSettings) like the others?

because i forgot to change it.

[fwereade, 2013-04-16 08:31:07]

LGTM with due consideration given to the following:

https://codereview.appspot.com/8761045/diff/2001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/8761045/diff/2001/state/megawatcher.go#newcode147
state/megawatcher.go:147: info.Config = s.disk
s.Map() or whatever it is? just on the basis of good hygiene? ;)

https://codereview.appspot.com/8761045/diff/2001/state/megawatcher.go#newcode306
state/megawatcher.go:306: info0 := store.Get(parentId)
On 2013/04/16 00:01:42, thumper wrote:
> Sorry, but info0 is a horrible name.  What is it actually representing?

Less so than it looks. It's an interface known to be wrapping a
*params.SomethingInfo; we want to give it a similar-but-different name so we can
give the known-typed version the more appropriate name of "info". The trailing 0
is ...maybe not necessarily *idiomatic* ...but I think it's unsurprising in
context, in the same way that python builtin collisions are usually evaded by
means of a trailing _.

Arguments re "SomethingInfo" are valid but orthogonal, I think: once you have a
*Info type, "info" becomes a meaningful name.

https://codereview.appspot.com/8761045/diff/2001/state/megawatcher.go#newcode335
state/megawatcher.go:335: func (a *backingSettings) mongoId() interface{} {
On 2013/04/16 00:01:42, thumper wrote:
> Why is this not (s *backingSettings) like the others?
+1

https://codereview.appspot.com/8761045/diff/2001/state/megawatcher_internal_t...
File state/megawatcher_internal_test.go (right):

https://codereview.appspot.com/8761045/diff/2001/state/megawatcher_internal_t...
state/megawatcher_internal_test.go:442: // the wordpress revision in the testing
repo.
maybe add something like: "It doesn't point to a real charm: its only salient
feature is that the URL is different to the one we're about to set."

Not sure that's very elegant, but I had to think about this for a bit.

[dimitern, 2013-04-16 12:33:42]

LGTM with one comment.

https://codereview.appspot.com/8761045/diff/2001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/8761045/diff/2001/state/megawatcher.go#newcode336
state/megawatcher.go:336: panic("cannot find mongo id from settings document")
why panic? provided you have the service and charm url, you can construct a
global key to the settings doc..

[rog, 2013-04-16 12:35:29]

https://codereview.appspot.com/8761045/diff/2001/state/megawatcher.go
File state/megawatcher.go (right):

https://codereview.appspot.com/8761045/diff/2001/state/megawatcher.go#newcode336
state/megawatcher.go:336: panic("cannot find mongo id from settings document")
On 2013/04/16 12:33:42, dimitern wrote:
> why panic? provided you have the service and charm url, you can construct a
> global key to the settings doc..

we don't have either of these things here.

[rog, 2013-04-16 14:58:13]

Please take a look.

[fwereade, 2013-04-16 15:09:28]

LGTM, much nicer

https://codereview.appspot.com/8761045/diff/11001/state/settings.go
File state/settings.go (right):

https://codereview.appspot.com/8761045/diff/11001/state/settings.go#newcode208
state/settings.go:208: // key. It returns the settings and the current rxnRevno.
s/rxn/txn/

[rog, 2013-04-16 15:11:13]

*** Submitted:

state: service config settings in AllWatcher

This does not provide default settings, so the
client needs to be intelligent about interpreting
the information, but that is possibly a reasonable
thing (saves network bandwidth - the client
has access to the charm metadata information).

R=thumper, fwereade, dimitern
CC=
https://codereview.appspot.com/8761045