Please take a look.


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 03:07:14, axw wrote:
> 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

Done. I confused my working branch with trunk - facepalm.

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"
On 2014/03/13 03:07:14, axw wrote:
> dummy will already be imported by the base test suite, but you should
import
> local here.

Done. Yeah, like in the line above - second facepalm.

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{}
On 2014/03/13 03:07:14, axw wrote:
> You need to initialise the map, like so:
>      secretAttrs := make(map[string]interface{})

Done.

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

Done. Strange, I guess not. Passes now.

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"
On 2014/03/13 03:07:14, axw wrote:
> s/vaidates/validates/

Done.

https://codereview.appspot.com/70190050/diff/80001/juju/conn_test.go#newcode154
juju/conn_test.go:154: // setting.
On 2014/03/13 03:07:14, axw wrote:
> 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.

Done.

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.
On 2014/03/13 03:07:14, axw wrote:
> ^^

> Adding the test could actually be useful here.

Done.

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)
On 2014/03/13 03:07:14, axw wrote:
> 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.

Done.

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
On 2014/03/13 03:07:14, axw wrote:
> Still not reinstated... did you forget to commit something?

Done.

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")
On 2014/03/13 03:07:14, axw wrote:
> 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.

Done.

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:
On 2014/03/13 03:07:14, axw wrote:
> Please add a test that exercises the new "additionalValidation"
function of
> UpdateEnvironConfig.

Done.

https://codereview.appspot.com/70190050/diff/80001/state/state_test.go#oldcode1762
state/state_test.go:1762: cfg, err := s.State.EnvironConfig()
On 2014/03/13 03:07:14, axw wrote:
> 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.

Done. Test is replaced.

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 {
On 2014/03/13 03:07:14, axw wrote:
> 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.

Done.

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)
On 2014/03/13 03:07:14, axw wrote:
> Why are all these tests deleted? We can still have an invalid config
at the
> moment.

Done.

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"`)
On 2014/03/13 03:07:14, axw wrote:
> This needs to do what it was doing before. Nil policy.

Done. Wow. I did a lot of really stupid things.

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