Code review comment for lp:~waigani/juju-core/get-oldConfig-from-state

Getting there.

https://codereview.appspot.com/70190050/diff/60001/state/apiserver/keymanager/keymanager.go
File state/apiserver/keymanager/keymanager.go (left):

https://codereview.appspot.com/70190050/diff/60001/state/apiserver/keymanager/keymanager.go#oldcode311
state/apiserver/keymanager/keymanager.go:311: // For now, authorised
keys are global, common to all users.
On 2014/03/13 01:02:03, waigani wrote:
> On 2014/03/10 03:41:54, axw wrote:
> > This comment didn't need to be deleted

> I'm confused. The comment is still there on line 312?

Huh, I must've been on crack. Never mind.

https://codereview.appspot.com/70190050/diff/60001/worker/environ_test.go
File worker/environ_test.go (left):

https://codereview.appspot.com/70190050/diff/60001/worker/environ_test.go#oldcode49
worker/environ_test.go:49: // Create an invalid config by taking the
current config and
On 2014/03/13 01:02:03, waigani wrote:
> On 2014/03/10 03:41:54, axw wrote:
> > This shouldn't go until UpdateEnvironConfig is completely fixed.

> I see that TestInvalidConfig is no longer in trunk, so I assume to is
fine to
> leave this deleted?

I just pulled trunk and it is still there. Not sure what you're looking
at...
http://bazaar.launchpad.net/~go-bot/juju-core/trunk/view/head:/worker/environ_test.go#L48

https://codereview.appspot.com/70190050/diff/60001/worker/machineenvironmentworker/machineenvironmentworker_test.go
File worker/machineenvironmentworker/machineenvironmentworker_test.go
(right):

https://codereview.appspot.com/70190050/diff/60001/worker/machineenvironmentworker/machineenvironmentworker_test.go#newcode128
worker/machineenvironmentworker/machineenvironmentworker_test.go:128:
attrs := map[string]interface{}{}
On 2014/03/13 01:02:03, waigani wrote:
> On 2014/03/10 03:41:54, axw wrote:
> > The conventional way to write this is
> > attrs := make(map[string]interface{})

> Ah. There are a lot of map[string]interface{}{} around (from my code
and
> others). Shall I put off changing these to another day/branch?

No problems, just leave them.

https://codereview.appspot.com/70190050/diff/80001/cmd/juju/environment_test.go
File cmd/juju/environment_test.go (right):

https://codereview.appspot.com/70190050/diff/80001/cmd/juju/environment_test.go#newcode14
cmd/juju/environment_test.go:14: _
"launchpad.net/juju-core/provider/dummy"
dummy will already be imported by the base test suite, but you should
import local here.

https://codereview.appspot.com/70190050/diff/80001/environs/statepolicy.go
File environs/statepolicy.go (right):

https://codereview.appspot.com/70190050/diff/80001/environs/statepolicy.go#newcode43
environs/statepolicy.go:43: if p, ok :=
provider.(state.ConfigValidator); ok {
No need for the assertion here, this is always going to be true for
Provider. Just go ahead and "return p, nil".

https://codereview.appspot.com/70190050/diff/80001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/70190050/diff/80001/juju/conn.go#newcode146
juju/conn.go:146: var secretAttrs map[string]interface{}
You need to initialise the map, like so:
     secretAttrs := make(map[string]interface{})

https://codereview.appspot.com/70190050/diff/80001/juju/conn.go#newcode153
juju/conn.go:153: secretAttrs[k] = v
This will panic, since you didn't initialise the map. I guess you didn't
run the tests after this change?

https://codereview.appspot.com/70190050/diff/80001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/70190050/diff/80001/juju/conn_test.go#newcode153
juju/conn_test.go:153: // UpdateEnvironConfig vaidates the config and
resets the "secret"
s/vaidates/validates/

https://codereview.appspot.com/70190050/diff/80001/juju/conn_test.go#newcode154
juju/conn_test.go:154: // setting.
In normal circumstances it shouldn't be possible, but this is
specifically for a test. Not removing it completely misses the point of
the test: that if the attributes *aren't* in state, they will be added.
This is testing compatibility with legacy code.

Please put this back to how it was, using a nil policy to override the
removal prevention.

https://codereview.appspot.com/70190050/diff/80001/state/api/common/testing/environwatcher.go
File state/api/common/testing/environwatcher.go (right):

https://codereview.appspot.com/70190050/diff/80001/state/api/common/testing/environwatcher.go#newcode91
state/api/common/testing/environwatcher.go:91: // TODO [waigani] test
removing an attribute.
^^

Adding the test could actually be useful here.

https://codereview.appspot.com/70190050/diff/80001/state/apiserver/keymanager/keymanager.go
File state/apiserver/keymanager/keymanager.go (right):

https://codereview.appspot.com/70190050/diff/80001/state/apiserver/keymanager/keymanager.go#newcode151
state/apiserver/keymanager/keymanager.go:151: err :=
api.state.UpdateEnvironConfig(attrs, nil, nil)
Can you please add a TODO to pass a validator that ensures the keys
haven't changed underfoot? This should have a sort of higher-level
transaction loop.

https://codereview.appspot.com/70190050/diff/80001/state/state.go
File state/state.go (left):

https://codereview.appspot.com/70190050/diff/80001/state/state.go#oldcode265
state/state.go:265: // TODO(axw) 2013-12-6 #1167616
Still not reinstated... did you forget to commit something?

https://codereview.appspot.com/70190050/diff/80001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/70190050/diff/80001/state/state.go#newcode283
state/state.go:283: return fmt.Errorf("both updateAttrs and removeAttrs
cannont be empty")
I'd be tempted just to return nil here, since there's nothing to do. It
could be easy to get into the situation where you compute a bunch of
changes to be made (add/modify/remove attributes) and then call
UpdateEnvironConfig. It seems onerous to have to check that you've got
no changes to make in each place.

https://codereview.appspot.com/70190050/diff/80001/state/state_test.go
File state/state_test.go (left):

https://codereview.appspot.com/70190050/diff/80001/state/state_test.go#oldcode1266
state/state_test.go:1266:
Please add a test that exercises the new "additionalValidation" function
of UpdateEnvironConfig.

https://codereview.appspot.com/70190050/diff/80001/state/state_test.go#oldcode1762
state/state_test.go:1762: cfg, err := s.State.EnvironConfig()
Remind me why this was deleted please? It's still possible for someone
to come along and muck up the attributes in Mongo (e.g. with an buggy
script that modified the db directly), just not from within Juju.

https://codereview.appspot.com/70190050/diff/80001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/70190050/diff/80001/state/state_test.go#newcode1261
state/state_test.go:1261: for k, v := range attrs {
You've changed the test a bit. It's now checking that the modified
attributes are modified, but no longer checking that the existing
attributes are unmodified, and that no additional attributes were added.

I think what you should do is get the config first, do an
UpdateEnvironConfig, merge the modified attributes with the existing
config and then ensure that EnvironConfig() DeepEquals the merged
config.

https://codereview.appspot.com/70190050/diff/80001/worker/provisioner/provisioner_test.go
File worker/provisioner/provisioner_test.go (left):

https://codereview.appspot.com/70190050/diff/80001/worker/provisioner/provisioner_test.go#oldcode460
worker/provisioner/provisioner_test.go:460: defer stop(c, p)
Why are all these tests deleted? We can still have an invalid config at
the moment.

https://codereview.appspot.com/70190050/diff/80001/worker/provisioner/provisioner_test.go
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/70190050/diff/80001/worker/provisioner/provisioner_test.go#newcode123
worker/provisioner/provisioner_test.go:123: c.Assert(err,
gc.ErrorMatches, `no registered provider for "unknown"`)
This needs to do what it was doing before. Nil policy.

https://codereview.appspot.com/70190050/