state: changed environ config to be config.Config

state: changed environ config to be config.Config

state.EnvironConfig() and the according watcher now
return environs/config.Config. Additionally the new
function state.SetEnvironConfig() replaces the current
configuration. Depending code - mostly tests - has 
been changed.

https://code.launchpad.net/~themue/juju-core/go-state-change-environ-config/+merge/121910

(do not edit description out of merge proposal)

[TheMue, 2012-08-29 17:35:49]

Please take a look.

[niemeyer, 2012-08-29 19:53:49]

https://codereview.appspot.com/6493055/diff/1/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6493055/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:169: cfgAttrs := cfg.AllAttrs()
This logic is bogus. We don't need to validate the configuration because we now
have a config.Config. If it was invalid, we wouldn't have the value in the first
place because config.New would refuse to create it.

https://codereview.appspot.com/6493055/diff/1/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/6493055/diff/1/juju/conn.go#newcode85
juju/conn.go:85: // TODO(mue) Is this comment still ok?
Yes, the comment is right, and the code is still wrong. Please drop the new
TODO.

https://codereview.appspot.com/6493055/diff/1/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/6493055/diff/1/juju/conn_test.go#newcode115
juju/conn_test.go:115: c.Assert(ok, Equals, false)
c.Assert(cfg.UnknownAttrs()["secret"], IsNil)

https://codereview.appspot.com/6493055/diff/1/juju/conn_test.go#newcode133
juju/conn_test.go:133: c.Assert(secret, Equals, "pork")
c.Assert(cfg.UnknownAttrs()["secret"], Equals, "pork")

https://codereview.appspot.com/6493055/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6493055/diff/1/state/state.go#newcode99
state/state.go:99: // environment with the attributes of the passed
configuration.
As we talked, this should be called SetEnvironConfig, and it should replace the
configuration rather than update it. The logic below is doing neither (Update
with a full configuration doesn't update.. it replaces everything in the full
configuration, and will leave arbitrarily unknown fields behind, which is an
unreasonable thing to do).

Please note that we don't even need to *read* the node.

https://codereview.appspot.com/6493055/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6493055/diff/1/state/state_test.go#newcode107
state/state_test.go:107: change map[string]interface{}
A slice of an in-place struct with a single field!? How about a slice? :-)

var enironmentWatchTests = []attrs{
        {
                "foo": "bar",
        }, {
                ...
        },
}

https://codereview.appspot.com/6493055/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6493055/diff/1/state/watcher.go#newcode113
state/watcher.go:113: // EnvironConfigWatcher observes changes to the environ
... to the environment configuration.

https://codereview.appspot.com/6493055/diff/1/state/watcher.go#newcode120
state/watcher.go:120: // newEnvironConfigWatcher creates and starts a new
environ config
"environment configuration"; this isn't a type nor a variable name.

https://codereview.appspot.com/6493055/diff/1/state/watcher.go#newcode132
state/watcher.go:132: // environ config when a change is detected. Note that
multiple
Ditto.

https://codereview.appspot.com/6493055/diff/1/state/watcher.go#newcode139
state/watcher.go:139: // A non-existent node is treated as an empty node.
If we have an empty node, we can return right away because we know it is
invalid.

https://codereview.appspot.com/6493055/diff/1/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6493055/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:58: fw.environ, err =
environs.NewFromAttrs(config.AllAttrs())
See environs.New.

https://codereview.appspot.com/6493055/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:76: config, err := config.New(change.AllAttrs())
Your mindful consideration of proposed changes would be appreciated.

https://codereview.appspot.com/6493055/diff/1/worker/firewaller/firewaller_te...
File worker/firewaller/firewaller_test.go (right):

https://codereview.appspot.com/6493055/diff/1/worker/firewaller/firewaller_te...
worker/firewaller/firewaller_test.go:106: env, err :=
environs.NewFromAttrs(cfg.AllAttrs())
Needs fixing.

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:58: p.environ, err =
environs.NewFromAttrs(config.AllAttrs())
Needs fixing.

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:92: config, err :=
config.New(change.AllAttrs())
Needs fixing.

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner_...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_test.go:23: coretesting.ZkConnSuite
If you introduce a second suite, you must write all the setup and teardown
methods, and they must call both suites.

That said, why are you introducing a second suite with Zk here? JujuConnSuite is
supposed to have Zk already, since it's a *Conn*, right?

This seems crackful.

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_test.go:47: environ, _, err :=
s.ZkConn.Get("/environment")
Grab the environment configuration itself. There's no reason to using zk for
that.

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_test.go:62: _, err :=
s.ZkConn.Set("/environment", s.environ, -1)
No reason to use zk for that either.

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_test.go:166: // Twiddle with the environment
configuration.
This test makes little sense to me. There's nothing being asserted about the
state of the provisioner at the end of the test. How could we test that the
provisioner has seen the new environment?

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_test.go:171: cfg, err = config.New(cfgAttrs)
c.Assert(err, IsNil)

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_test.go:175: cfg, err = config.New(cfgAttrs)
c.Assert(err, IsNil)

[TheMue, 2012-08-30 13:21:51]

Please take a look.

https://codereview.appspot.com/6493055/diff/1/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6493055/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:169: cfgAttrs := cfg.AllAttrs()
On 2012/08/29 19:53:49, niemeyer wrote:
> This logic is bogus. We don't need to validate the configuration because we
now
> have a config.Config. If it was invalid, we wouldn't have the value in the
first
> place because config.New would refuse to create it.

Done.

https://codereview.appspot.com/6493055/diff/1/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/6493055/diff/1/juju/conn.go#newcode85
juju/conn.go:85: // TODO(mue) Is this comment still ok?
On 2012/08/29 19:53:49, niemeyer wrote:
> Yes, the comment is right, and the code is still wrong. Please drop the new
> TODO.

Done.

https://codereview.appspot.com/6493055/diff/1/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/6493055/diff/1/juju/conn_test.go#newcode115
juju/conn_test.go:115: c.Assert(ok, Equals, false)
On 2012/08/29 19:53:49, niemeyer wrote:
> c.Assert(cfg.UnknownAttrs()["secret"], IsNil)

Done.

https://codereview.appspot.com/6493055/diff/1/juju/conn_test.go#newcode133
juju/conn_test.go:133: c.Assert(secret, Equals, "pork")
On 2012/08/29 19:53:49, niemeyer wrote:
> c.Assert(cfg.UnknownAttrs()["secret"], Equals, "pork")

Done.

https://codereview.appspot.com/6493055/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/6493055/diff/1/state/state.go#newcode99
state/state.go:99: // environment with the attributes of the passed
configuration.
On 2012/08/29 19:53:49, niemeyer wrote:
> As we talked, this should be called SetEnvironConfig, and it should replace
the
> configuration rather than update it. The logic below is doing neither (Update
> with a full configuration doesn't update.. it replaces everything in the full
> configuration, and will leave arbitrarily unknown fields behind, which is an
> unreasonable thing to do).
> 
> Please note that we don't even need to *read* the node.

It has been changed to update after a discussion with Roger and William (see IRC
logs of yesterday about 8:00).

https://codereview.appspot.com/6493055/diff/1/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/6493055/diff/1/state/state_test.go#newcode107
state/state_test.go:107: change map[string]interface{}
On 2012/08/29 19:53:49, niemeyer wrote:
> A slice of an in-place struct with a single field!? How about a slice? :-)
> 
> var enironmentWatchTests = []attrs{
>         {
>                 "foo": "bar",
>         }, {
>                 ...
>         },
> }

Ooops, indeed! Forgot to cleanup after start with multiple fields. :|

https://codereview.appspot.com/6493055/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/6493055/diff/1/state/watcher.go#newcode113
state/watcher.go:113: // EnvironConfigWatcher observes changes to the environ
On 2012/08/29 19:53:49, niemeyer wrote:
> ... to the environment configuration.

Done.

https://codereview.appspot.com/6493055/diff/1/state/watcher.go#newcode120
state/watcher.go:120: // newEnvironConfigWatcher creates and starts a new
environ config
On 2012/08/29 19:53:49, niemeyer wrote:
> "environment configuration"; this isn't a type nor a variable name.

Done.

https://codereview.appspot.com/6493055/diff/1/state/watcher.go#newcode132
state/watcher.go:132: // environ config when a change is detected. Note that
multiple
On 2012/08/29 19:53:49, niemeyer wrote:
> Ditto.

Done.

https://codereview.appspot.com/6493055/diff/1/state/watcher.go#newcode139
state/watcher.go:139: // A non-existent node is treated as an empty node.
On 2012/08/29 19:53:49, niemeyer wrote:
> If we have an empty node, we can return right away because we know it is
> invalid.

Done.

https://codereview.appspot.com/6493055/diff/1/worker/firewaller/firewaller.go
File worker/firewaller/firewaller.go (right):

https://codereview.appspot.com/6493055/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:58: fw.environ, err =
environs.NewFromAttrs(config.AllAttrs())
On 2012/08/29 19:53:49, niemeyer wrote:
> See environs.New.

Done.

https://codereview.appspot.com/6493055/diff/1/worker/firewaller/firewaller.go...
worker/firewaller/firewaller.go:76: config, err := config.New(change.AllAttrs())
On 2012/08/29 19:53:49, niemeyer wrote:
> Your mindful consideration of proposed changes would be appreciated.

First round concentrated on naive 1:1 replacement of ConfigNode with
config.Config and follow-up has been intended to do further changes based on the
change of state.EnvironConfig(), etc.

https://codereview.appspot.com/6493055/diff/1/worker/firewaller/firewaller_te...
File worker/firewaller/firewaller_test.go (right):

https://codereview.appspot.com/6493055/diff/1/worker/firewaller/firewaller_te...
worker/firewaller/firewaller_test.go:106: env, err :=
environs.NewFromAttrs(cfg.AllAttrs())
On 2012/08/29 19:53:49, niemeyer wrote:
> Needs fixing.

Done.

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner.go
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:58: p.environ, err =
environs.NewFromAttrs(config.AllAttrs())
On 2012/08/29 19:53:49, niemeyer wrote:
> Needs fixing.

Done.

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner....
worker/provisioner/provisioner.go:92: config, err :=
config.New(change.AllAttrs())
On 2012/08/29 19:53:49, niemeyer wrote:
> Needs fixing.

Done.

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner_...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_test.go:23: coretesting.ZkConnSuite
On 2012/08/29 19:53:49, niemeyer wrote:
> If you introduce a second suite, you must write all the setup and teardown
> methods, and they must call both suites.
> 
> That said, why are you introducing a second suite with Zk here? JujuConnSuite
is
> supposed to have Zk already, since it's a *Conn*, right?
> 
> This seems crackful.

Yes, not nice. Missed the function testing.ZkConnect() which is now used. I've
been too much focussed on the JujuConnSuite, which embeds ZkSuite to start ZK
but doesn't expose a ZK connection. But due to the function above I can open a
quick connection.

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_test.go:47: environ, _, err :=
s.ZkConn.Get("/environment")
On 2012/08/29 19:53:49, niemeyer wrote:
> Grab the environment configuration itself. There's no reason to using zk for
> that.

Done.

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_test.go:62: _, err :=
s.ZkConn.Set("/environment", s.environ, -1)
On 2012/08/29 19:53:49, niemeyer wrote:
> No reason to use zk for that either.

Done.

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_test.go:166: // Twiddle with the environment
configuration.
On 2012/08/29 19:53:49, niemeyer wrote:
> This test makes little sense to me. There's nothing being asserted about the
> state of the provisioner at the end of the test. How could we test that the
> provisioner has seen the new environment?

Sadly I don't know the original intention. The provisioner only updates the
environment configuration with p.environ.SetConfig() with each change.

I'll ask Dave about it.

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_test.go:171: cfg, err = config.New(cfgAttrs)
On 2012/08/29 19:53:49, niemeyer wrote:
> c.Assert(err, IsNil)

Done.

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_test.go:175: cfg, err = config.New(cfgAttrs)
On 2012/08/29 19:53:49, niemeyer wrote:
> c.Assert(err, IsNil)

Done.

[niemeyer, 2012-08-30 14:36:57]

LGTM, thanks.

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner_...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/6493055/diff/1/worker/provisioner/provisioner_...
worker/provisioner/provisioner_test.go:166: // Twiddle with the environment
configuration.
On 2012/08/30 13:21:51, TheMue wrote:
> On 2012/08/29 19:53:49, niemeyer wrote:
> > This test makes little sense to me. There's nothing being asserted about the
> > state of the provisioner at the end of the test. How could we test that the
> > provisioner has seen the new environment?
> 
> Sadly I don't know the original intention. The provisioner only updates the
> environment configuration with p.environ.SetConfig() with each change.
> 
> I'll ask Dave about it.

We've discussed this over IRC. Let's fix the test in a follow up CL.

[TheMue, 2012-08-30 14:59:35]

*** Submitted:

state: changed environ config to be config.Config

state.EnvironConfig() and the according watcher now
return environs/config.Config. Additionally the new
function state.SetEnvironConfig() replaces the current
configuration. Depending code - mostly tests - has 
been changed.

R=niemeyer
CC=
https://codereview.appspot.com/6493055