I haven't reviewed everything, this should keep you going for a while
though.

In general: we should not be dropping any tests that are not
*guaranteed* to never work. Since UpdateEnvironConfig is not completely
fixed yet, the tests should remain. This may involve hacking around
validation with a nil policy.


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

https://codereview.appspot.com/70190050/diff/60001/cmd/juju/environment.go#newcode166
cmd/juju/environment.go:166: return
conn.State.UpdateEnvironConfig(c.values, []string{})
Just nil is fine.

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

https://codereview.appspot.com/70190050/diff/60001/cmd/juju/environment_test.go#newcode173
cmd/juju/environment_test.go:173: c.Check(err, gc.ErrorMatches, `no
registered provider for "foo"`)
This has nothing to do with attribute mutability, so doesn't belong
here. Why not just change the value in the map to a real provider type?
You will need to make sure the provider is registered by importing its
package.

https://codereview.appspot.com/70190050/diff/60001/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/70190050/diff/60001/environs/config.go#newcode138
environs/config.go:138: func Provider(providerType string)
(EnvironProvider, error) {
Making cosmetic changes is fine, but it's best not to mix cosmetic
changes (or refactoring) with functional changes. Otherwise I'm
scrutinising everything to make sure there's no functional change, and
the review takes longer than necessary.

https://codereview.appspot.com/70190050/diff/60001/environs/testing/tools.go
File environs/testing/tools.go (right):

https://codereview.appspot.com/70190050/diff/60001/environs/testing/tools.go#newcode447
environs/testing/tools.go:447: err = st.UpdateEnvironConfig(attrs,
[]string{})
Just pass in the one attribute in a new map. FYI, you can make maps with
map literals like so:
   map[string]interface{}{"ssl-hostname-verification":
SSLHostnameVerification}

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

https://codereview.appspot.com/70190050/diff/60001/juju/conn.go#newcode155
juju/conn.go:155: return c.State.UpdateEnvironConfig(attrs, []string{})
Just pass secrets here, and don't bother updating attrs in the loop.

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

https://codereview.appspot.com/70190050/diff/60001/juju/conn_test.go#newcode152
juju/conn_test.go:152: // Removing the secret from state should not be
possible, as
Well, that's only true for the dummy provider. Real providers don't have
defaults for secrets. For a real provider you'd have a different
problem: validation would fail because a config without the secrets is
invalid.

In this test it makes sense to disable validation while you're removing
the attribute.

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

https://codereview.appspot.com/70190050/diff/60001/state/api/firewaller/state_test.go#newcode94
state/api/firewaller/state_test.go:94: // TODO [waigani] test removing
an attribute.
I don't think deferral is warranted here. Not sure it's really necessary
to add the test either, though: firewaller doesn't care about attributes
being removed.

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

https://codereview.appspot.com/70190050/diff/60001/state/apiserver/client/client.go#newcode775
state/apiserver/client/client.go:775: oldConfig, err :=
c.api.state.EnvironConfig()
This is a bit ugly, requiring another load. I wonder if we could pass an
additional validation function down to UpdateEnvironConfig...
Just a thought, not blocking.

https://codereview.appspot.com/70190050/diff/60001/state/apiserver/client/client.go#newcode785
state/apiserver/client/client.go:785: // TODO [waigani] Add a txn retry
loop
We have a standard format for TODOs:

// TODO(user) YYYY-MM-DD #issue
// A brief summary of the issue here.

Please file an issue if there's not one already (I think the one I
referred to in state.go is suitable, so just refer to that).

https://codereview.appspot.com/70190050/diff/60001/state/apiserver/client/client.go#newcode786
state/apiserver/client/client.go:786: return
c.api.state.UpdateEnvironConfig(args.Config, []string{})
The second argument can simply be nil.

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.
This comment didn't need to be deleted

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

https://codereview.appspot.com/70190050/diff/60001/state/state.go#oldcode265
state/state.go:265: // TODO(axw) 2013-12-6 #1167616
The TODO will not be resolved until the settings-haven't-changed
assertion is in place. Please do not delete it until then,.

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

https://codereview.appspot.com/70190050/diff/60001/state/state.go#newcode259
state/state.go:259: // DELETE ME
DELETE ME

https://codereview.appspot.com/70190050/diff/60001/state/state.go#newcode305
state/state.go:305: oldConfig, err := config.New(config.NoDefaults,
existingAttrs)
I think there's no reason not to inline settings.Map() here.

https://codereview.appspot.com/70190050/diff/60001/state/testing/config.go
File state/testing/config.go (right):

https://codereview.appspot.com/70190050/diff/60001/state/testing/config.go#newcode10
state/testing/config.go:10: // TODO [waigani] replace UpdateConfig with
UpdateEnvironConfig
Please either do it or create a TODO(waigaini) with accompanying issue.

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
This shouldn't go until UpdateEnvironConfig is completely fixed.

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

https://codereview.appspot.com/70190050/diff/60001/worker/firewaller/firewaller_test.go#oldcode135
worker/firewaller/firewaller_test.go:135: // has bootstrapped.
Winner!

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

https://codereview.appspot.com/70190050/diff/60001/worker/instancepoller/observer_test.go#oldcode51
worker/instancepoller/observer_test.go:51: oldType =
attrs["type"].(string)
Again, don't delete until UpdateEnvironConfig is fixed. Open a state
conn with nil validator here if you have to.

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{}{}
The conventional way to write this is
    attrs := make(map[string]interface{})

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