Please take a look. 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{}) On 2014/03/10 03:41:54, axw wrote: > Just nil is fine. Done. 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"`) On 2014/03/10 03:41:54, axw wrote: > 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. Done. 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) { On 2014/03/10 03:41:54, axw wrote: > 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. Sorry about that. This branch is actually three squashed together. These changes are in response to William's review of one of those branches. 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{}) On 2014/03/10 03:41:54, axw wrote: > Just pass secrets here, and don't bother updating attrs in the loop. Done. I couldn't cast or assert secrets (which is map[string]string) to map[string]interface{}, so I created a new var secretAttrs with that type instead. 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. On 2014/03/10 03:41:54, axw wrote: > 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. Done. TODO removed. 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. On 2014/03/10 03:41:54, axw wrote: > 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() On 2014/03/10 03:41:54, axw wrote: > 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. Done. UpdateEnvironConfig now takes an additionalValidation func 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 On 2014/03/10 03:41:54, axw wrote: > 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). Done. I also returned your TODO comment in state.go. Thanks for showing me the standard. 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{}) On 2014/03/10 03:41:54, axw wrote: > The second argument can simply be nil. Done. I changed all calls. 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/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? 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 On 2014/03/10 03:41:54, axw wrote: > The TODO will not be resolved until the settings-haven't-changed assertion is in > place. Please do not delete it until then,. Done. 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 On 2014/03/10 03:41:54, axw wrote: > DELETE ME Done. https://codereview.appspot.com/70190050/diff/60001/state/state.go#newcode305 state/state.go:305: oldConfig, err := config.New(config.NoDefaults, existingAttrs) On 2014/03/10 03:41:54, axw wrote: > I think there's no reason not to inline settings.Map() here. Done. 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 On 2014/03/10 03:41:54, axw wrote: > Please either do it or create a TODO(waigaini) with accompanying issue. Done. UpdateConfig replaced. 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/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? 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) On 2014/03/10 03:41:54, axw wrote: > Again, don't delete until UpdateEnvironConfig is fixed. Open a state conn with > nil validator here if you have to. Done. 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/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? https://codereview.appspot.com/70190050/