state: add WatchEnvironConfig

state: add WatchEnvironConfig

This change adds a watcher for the /environment key. The result 
returned from changes to this key is a *ConfigNode. The idea is 
the provisioning agent will watch this key, then convert the 
*ConfigNode into an Environ via a yet to be written facility.

Also included is a bug fix detected while testing and a few 
small tweaks to util.go.

https://code.launchpad.net/~dave-cheney/juju/go-environment/+merge/105375

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-05-10 19:24:22]

Please take a look.

[dave_cheney.net, 2012-05-10 22:25:28]

Please take a look.

[dave_cheney.net, 2012-05-10 23:29:05]

Please take a look.

[fwereade, 2012-05-14 13:08:46]

LGTM

https://codereview.appspot.com/6207050/diff/5001/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/6207050/diff/5001/state/watcher_test.go#newcod...
state/watcher_test.go:290: // make sure there is an /environment key
Maybe a spot more explanation of why we can assume it exists?

[dave_cheney.net, 2012-05-14 22:35:26]

Please take a look.

https://codereview.appspot.com/6207050/diff/5001/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/6207050/diff/5001/state/watcher_test.go#newcod...
state/watcher_test.go:290: // make sure there is an /environment key
On 2012/05/14 13:08:46, fwereade wrote:
> Maybe a spot more explanation of why we can assume it exists?

Done.

[niemeyer, 2012-05-14 23:17:20]

https://codereview.appspot.com/6207050/diff/6003/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/6207050/diff/6003/environs/config.go#newcode105
environs/config.go:105: environ, err := p.Open(attrs)
It's not clear to me why this is changing. We had schema validation logic that
was already able to validate environments without instantiating them, and had
logic that opened only the environment that is actually used. This seems good
and compatible with what was debated at UDS. It'd be bad to call Open on 20
environments from a configuration file when we want to use a single one of them.

The new change with Getter/Setter also seems to add a layer of complexity on top
of what we have, without really offering a nice API for us to deal with it. It's
also incomplete (there's a TODO above that breaks the code) and unrelated to the
watcher that is what is documented as being the topic of this branch and CL. I'm
certainly happy to have changes to that API improving things, but this isn't
feeling good.

Have you pushed something by mistake here?

https://codereview.appspot.com/6207050/diff/6003/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6207050/diff/6003/state/state.go#newcode16
state/state.go:16: zkEnvironmentPath = "/environment"
If it's a single const, the block syntax isn't so useful.

That said, it seems that this const is used in a single place. We don't need the
const if that's the case.

https://codereview.appspot.com/6207050/diff/6003/state/util.go
File state/util.go (right):

https://codereview.appspot.com/6207050/diff/6003/state/util.go#newcode91
state/util.go:91: cache: make(map[string]interface{}),
Why?

https://codereview.appspot.com/6207050/diff/6003/state/util.go#newcode221
state/util.go:221: for key, value := range kv {
Thanks for cleaning this up.

https://codereview.appspot.com/6207050/diff/6003/state/util.go#newcode265
state/util.go:265: // If in is nil, out will be an empty map.
Rather than doing that and changing below, it'd probably be nicer to have
something like:

if in != nil {
    out = make...
}

this will give us a nil map, and no allocations, in case it's empty, and offer
just as nice an API.

https://codereview.appspot.com/6207050/diff/6003/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/6207050/diff/6003/state/watcher_test.go#newcod...
state/watcher_test.go:286: }, any{"providor": "aws", "secret": "shhh"}},
This can be made more readable, in the same direction of the other review when
we talked about formatting of such tables.

[dave_cheney.net, 2012-05-14 23:22:09]

I cannot explain why the environs/ files leaked into this proposal. I need to
brain harder with lbox.

[dave_cheney.net, 2012-05-14 23:41:58]

Please take a look.

https://codereview.appspot.com/6207050/diff/6003/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/6207050/diff/6003/environs/config.go#newcode105
environs/config.go:105: environ, err := p.Open(attrs)
Yes, sorry.

https://codereview.appspot.com/6207050/diff/6003/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6207050/diff/6003/state/state.go#newcode16
state/state.go:16: zkEnvironmentPath = "/environment"
Re the block,
https://code.launchpad.net/~dave-cheney/juju/go-machine-watcher-refactor will
add additional elements to this block.

re: the use of a constant, this was following the pattern at the request of
frank with the /machines watcher. I have no preference for the use of a constant
or not.

On 2012/05/14 23:17:21, niemeyer wrote:
> If it's a single const, the block syntax isn't so useful.
> 
> That said, it seems that this const is used in a single place. We don't need
the
> const if that's the case.

https://codereview.appspot.com/6207050/diff/6003/state/util.go
File state/util.go (right):

https://codereview.appspot.com/6207050/diff/6003/state/util.go#newcode91
state/util.go:91: cache: make(map[string]interface{}),
On 2012/05/14 23:17:21, niemeyer wrote:
> Why?

because otherwise the cache is empty when the test first calls config.Set().

Most of the changes in here have been superseded by
https://code.launchpad.net/~rogpeppe/juju/go-clean-up-config-node

https://codereview.appspot.com/6207050/diff/6003/state/util.go#newcode265
state/util.go:265: // If in is nil, out will be an empty map.
See 1st comment

On 2012/05/14 23:17:21, niemeyer wrote:
> Rather than doing that and changing below, it'd probably be nicer to have
> something like:
> 
> if in != nil {
>     out = make...
> }
> 
> this will give us a nil map, and no allocations, in case it's empty, and offer
> just as nice an API.

https://codereview.appspot.com/6207050/diff/6003/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/6207050/diff/6003/state/watcher_test.go#newcod...
state/watcher_test.go:286: }, any{"providor": "aws", "secret": "shhh"}},
Certainly, this is the default go fmt formatting, I will reformat

[gustavo_niemeyer.net, 2012-05-14 23:42:08]

On Mon, May 14, 2012 at 8:22 PM,  <dave@cheney.net> wrote:
> I cannot explain why the environs/ files leaked into this proposal. I
> need to brain harder with lbox.

It's in the branch. lbox will just push the branch diff up.


gustavo @ http://niemeyer.net

[niemeyer, 2012-05-15 00:18:56]

Thanks, almost there.

https://codereview.appspot.com/6207050/diff/5004/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6207050/diff/5004/state/state.go#newcode16
state/state.go:16: zkEnvironmentPath = "/environment"
Please put this in a single line right above its use case below. It's fine to
have a block when the additional variables get introduced, if it makes sense to
have them in the same location. Before that it's just noise that we can easily
avoid.

https://codereview.appspot.com/6207050/diff/5004/state/state.go#newcode69
state/state.go:69: // WatchEnvironment watches for changes to the /environment
key
We shouldn't document things in terms of the ZooKeeper structure. It's just an
implementation detail that is not relevant to the consumers of the state
package. Instead, we should document the state API in terms of what it means
semantically for juju. For example:

// WatchEnvironConfig returns a watcher for observing
// changes to the environment configuration.

https://codereview.appspot.com/6207050/diff/5004/state/watcher_test.go
File state/watcher_test.go (right):

https://codereview.appspot.com/6207050/diff/5004/state/watcher_test.go#newcod...
state/watcher_test.go:275: _, err := config.Write()
These two lines are being repeated on all functions. The logic may be simplified
further to avoid this. In fact, it looks like the only thing changing in all of
these functions is a key and a value, so we don't even need a function.

https://codereview.appspot.com/6207050/diff/5004/state/watcher_test.go#newcod...
state/watcher_test.go:290: config.Update(any{"providor": "aws"})
Isn't this just Set("providor", "aws")?

[dave_cheney.net, 2012-05-15 00:52:27]

Please take a look.

https://codereview.appspot.com/6207050/diff/5004/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6207050/diff/5004/state/state.go#newcode16
state/state.go:16: zkEnvironmentPath = "/environment"
On 2012/05/15 00:18:57, niemeyer wrote:
> Please put this in a single line right above its use case below. It's fine to
> have a block when the additional variables get introduced, if it makes sense
to
> have them in the same location. Before that it's just noise that we can easily
> avoid.

Done.

https://codereview.appspot.com/6207050/diff/5004/state/state.go#newcode69
state/state.go:69: // WatchEnvironment watches for changes to the /environment
key
SGTM. Done

[rog, 2012-05-15 08:44:38]

LGTM

[niemeyer, 2012-05-15 14:14:59]

LGTM with the minor fix on the function name.

https://codereview.appspot.com/6207050/diff/6004/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6207050/diff/6004/state/state.go#newcode69
state/state.go:69: func (s *State) WatchEnvironment() *ConfigWatcher {
The name of the function has to change to match the documentation as well. I
suggest WatchEnvironConfig rather than WatchEnvironment (we're not really
watching the *whole* environment).

[dave_cheney.net, 2012-05-15 22:40:07]

Please take a look.

[dave_cheney.net, 2012-05-15 22:43:18]

Going to submit now.

On Wed, May 16, 2012 at 8:40 AM, <dave@cheney.net> wrote:

> Please take a look.
>
>
https://codereview.appspot.**com/6207050/<https://codereview.appspot.com/6207...
>

[dave_cheney.net, 2012-05-15 22:47:04]

*** Submitted:

state: add WatchEnvironConfig

This change adds a watcher for the /environment key. The result 
returned from changes to this key is a *ConfigNode. The idea is 
the provisioning agent will watch this key, then convert the 
*ConfigNode into an Environ via a yet to be written facility.

Also included is a bug fix detected while testing and a few 
small tweaks to util.go.

R=fwereade, niemeyer, gustavo@niemeyer.net, rog
CC=
https://codereview.appspot.com/6207050