Update and Validate Environ Config in State

Update and Validate Environ Config in State

The main business logic happens in state/state.go. The bulk of the other
changes are mechanical i.e. updating method calls. There are a few things going on:

-Explicitly add/update or remove attributes with state.UpdateEnvironConf, which
replaces state.SetEnvironConf.
- Add ConfigValidator to state.Policy interface.
- Validate environment configuration within state.
    - build new config from UpdateEnvironConf args, get old config from state.
    - pass both configs to st.validate which use the environ policy
	ConfigValidator to get the correct validation method.
    - use the validated config to make the actual updates in state.
- Update or delete tests that rely on an invalid configuration being set, as
    this is now not possible.

A follow up branch will use txn and a retry loop to address a race condition in
updating state config to ensure that concurrent updates to state config do not corrupt one
another.

https://code.launchpad.net/~waigani/juju-core/get-oldConfig-from-state/+merge/209569

(do not edit description out of merge proposal)

[axw, 2014-03-06 04:19:16]

Some preliminary comments. I've only really looked at state.go.

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

https://codereview.appspot.com/70190050/diff/1/state/state.go#oldcode282
state/state.go:282: _, err = settings.Write()
Write isn't really good enough here; this does not address the deleted TODO. The
update to settings should assert that there has been no change to settings since
st.EnvironConfig() was called; otherwise the validation is not complete.

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

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode259
state/state.go:259: func (st *State) VaidateUpdateEnvironConfig(updateAttrs
map[string]interface{}, removeAttrs []string, oldConfig *config.Config) error {
s/Vaidate/Validate/
also, doesn't need to be exported does it?

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode264
state/state.go:264: // Build new config with attributes to add/update
You can get rid of the comments, if statement, forward declared newConfig/err
vars, and just replace with:

    newConfig, err := oldConfig.Apply(updateAttrs)
    if err != nil {
        return err
    }

Apply doesn't care if updateAttrs is nil or empty.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode274
state/state.go:274: if len(removeAttrs) != 0 {
And then since newConfig exists unconditionally:

    newConfig, err = newConfig.Remove(removeAttrs)
    if err != nil {
        return err
    }

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode304
state/state.go:304: func (st *State) SetEnvironConfig(attrs
map[string]interface{}) error {
Why do we still need SetEnvironConfig? This still has the old race conditions.
*If* it's needed, then SetEnvironConfig and UpdateEnvironConfig should use a
common method that takes a *state.Settings as input and does the
no-concurrent-change check on that.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode327
state/state.go:327: oldConfig, err := st.EnvironConfig()
This does a readSettings, so you're reading settings twice. I think it'd be
preferable to just do a readSettings and use its Map() to create a config.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode333
state/state.go:333: err = st.VaidateUpdateEnvironConfig(updateAttrs,
removeAttrs, oldConfig)
Since ValidateUpdateEnvironConfig updates the config with attributes to
validate, I wonder if it's worthwhile even having a separate method. You're
duplicating work here by updating settings.

I think it would be simpler to just:
   1. get settings, and create a config from them
   2. apply changes to get a new config, check validity
   3. do something *like* replaceSettingsOp (see settings.go), but passing in
the *state.Settings

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode351
state/state.go:351: // Add/Update attributes.
These comments aren't really useful, the code speaks for itself.

[waigani, 2014-03-06 08:17:47]

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

https://codereview.appspot.com/70190050/diff/1/state/state.go#oldcode282
state/state.go:282: _, err = settings.Write()
On 2014/03/06 04:19:17, axw wrote:
> Write isn't really good enough here; this does not address the deleted TODO.
The
> update to settings should assert that there has been no change to settings
since
> st.EnvironConfig() was called; otherwise the validation is not complete.

fwereade wanted a txn retry loop, ensuring the underlying settings had not been
changed, in client.EnvironmentSet
(https://codereview.appspot.com/63790045/diff/40001/state/apiserver/client/cli...).
As this is a WIP, I have not done it yet. I can make a TODO in
client.EnvironmentSet and write the txn retry loop in a follow up branch?

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

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode259
state/state.go:259: func (st *State) VaidateUpdateEnvironConfig(updateAttrs
map[string]interface{}, removeAttrs []string, oldConfig *config.Config) error {
On 2014/03/06 04:19:17, axw wrote:
> s/Vaidate/Validate/
> also, doesn't need to be exported does it?

Good catch!

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode264
state/state.go:264: // Build new config with attributes to add/update
On 2014/03/06 04:19:17, axw wrote:
> You can get rid of the comments, if statement, forward declared newConfig/err
> vars, and just replace with:
> 
>     newConfig, err := oldConfig.Apply(updateAttrs)
>     if err != nil {
>         return err
>     }
> 
> Apply doesn't care if updateAttrs is nil or empty.

Done.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode274
state/state.go:274: if len(removeAttrs) != 0 {
On 2014/03/06 04:19:17, axw wrote:
> And then since newConfig exists unconditionally:
> 
>     newConfig, err = newConfig.Remove(removeAttrs)
>     if err != nil {
>         return err
>     }

Done.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode304
state/state.go:304: func (st *State) SetEnvironConfig(attrs
map[string]interface{}) error {
On 2014/03/06 04:19:17, axw wrote:
> Why do we still need SetEnvironConfig? This still has the old race conditions.
> *If* it's needed, then SetEnvironConfig and UpdateEnvironConfig should use a
> common method that takes a *state.Settings as input and does the
> no-concurrent-change check on that.

It was needed by testing.ChangeEnvironConfig. ChangeEnvironConfig is only used
in two places, but it is actually easier to use UpdateEnvironConfig directly
instead. So I would be in favour of deleting SetEnvironConfig and
ChangeEnvironConfig.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode327
state/state.go:327: oldConfig, err := st.EnvironConfig()
On 2014/03/06 04:19:17, axw wrote:
> This does a readSettings, so you're reading settings twice. I think it'd be
> preferable to just do a readSettings and use its Map() to create a config.

Done.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode333
state/state.go:333: err = st.VaidateUpdateEnvironConfig(updateAttrs,
removeAttrs, oldConfig)
On 2014/03/06 04:19:17, axw wrote:
> Since ValidateUpdateEnvironConfig updates the config with attributes to
> validate, I wonder if it's worthwhile even having a separate method. You're
> duplicating work here by updating settings.
> 
> I think it would be simpler to just:
>    1. get settings, and create a config from them
>    2. apply changes to get a new config, check validity
>    3. do something *like* replaceSettingsOp (see settings.go), but passing in
> the *state.Settings

Do you think something *like* replaceSettingsOp should happen within
state.UpdateEnvironConfig or client.EnvironmentSet?

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode351
state/state.go:351: // Add/Update attributes.
On 2014/03/06 04:19:17, axw wrote:
> These comments aren't really useful, the code speaks for itself.

Done.

[axw, 2014-03-06 08:34:44]

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

https://codereview.appspot.com/70190050/diff/1/state/state.go#oldcode282
state/state.go:282: _, err = settings.Write()
On 2014/03/06 08:17:48, waigani wrote:
> On 2014/03/06 04:19:17, axw wrote:
> > Write isn't really good enough here; this does not address the deleted TODO.
> The
> > update to settings should assert that there has been no change to settings
> since
> > st.EnvironConfig() was called; otherwise the validation is not complete.
> 
> fwereade wanted a txn retry loop, ensuring the underlying settings had not
been
> changed, in client.EnvironmentSet
>
(https://codereview.appspot.com/63790045/diff/40001/state/apiserver/client/cli...).
> As this is a WIP, I have not done it yet. I can make a TODO in
> client.EnvironmentSet and write the txn retry loop in a follow up branch?
> 

The assertion is a prerequisite of the looping. The assertion is what ensures
that the settings haven't been changed concurrently. If the settings are changed
underfoot, then the transaction fails with txn.ErrAborted.

The assertion must happen here, whereas looping may happen here or in the
caller. There's basically no point in making any of these changes unless that
assertion is in place.

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

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode304
state/state.go:304: func (st *State) SetEnvironConfig(attrs
map[string]interface{}) error {
On 2014/03/06 08:17:48, waigani wrote:
> On 2014/03/06 04:19:17, axw wrote:
> > Why do we still need SetEnvironConfig? This still has the old race
conditions.
> > *If* it's needed, then SetEnvironConfig and UpdateEnvironConfig should use a
> > common method that takes a *state.Settings as input and does the
> > no-concurrent-change check on that.
> 
> It was needed by testing.ChangeEnvironConfig. ChangeEnvironConfig is only used
> in two places, but it is actually easier to use UpdateEnvironConfig directly
> instead. So I would be in favour of deleting SetEnvironConfig and
> ChangeEnvironConfig.
> 

ChangeEnvironConfig can be modified to compute a delta and pass it into
UpdateEnvironConfig. I know it's a bit arse about, but I don't think we should
be exposing ourselves to bugs just to simplify a test. If you can just get rid
of ChangeEnvironConfig, though, that may be better.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode333
state/state.go:333: err = st.VaidateUpdateEnvironConfig(updateAttrs,
removeAttrs, oldConfig)
On 2014/03/06 08:17:48, waigani wrote:
> On 2014/03/06 04:19:17, axw wrote:
> > Since ValidateUpdateEnvironConfig updates the config with attributes to
> > validate, I wonder if it's worthwhile even having a separate method. You're
> > duplicating work here by updating settings.
> > 
> > I think it would be simpler to just:
> >    1. get settings, and create a config from them
> >    2. apply changes to get a new config, check validity
> >    3. do something *like* replaceSettingsOp (see settings.go), but passing
in
> > the *state.Settings
> 
> Do you think something *like* replaceSettingsOp should happen within
> state.UpdateEnvironConfig or client.EnvironmentSet?

Certainly in state. How would you propose to do it outside anyway?

I suspect what William meant is that UpdateEnvironConfig should be retried for
some number of attempts until it returns an error != mgo/txn/ErrAborted; better
to check with William.

[waigani, 2014-03-07 23:28:49]

Please take a look.

[waigani, 2014-03-07 23:36:00]

Please take a look.

[waigani, 2014-03-07 23:45:07]

Please take a look.

[axw, 2014-03-10 03:41:53]

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#ne...
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....
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#...
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...
File state/api/firewaller/state_test.go (right):

https://codereview.appspot.com/70190050/diff/60001/state/api/firewaller/state...
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/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/70190050/diff/60001/state/apiserver/client/cli...
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/cli...
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/cli...
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...
File state/apiserver/keymanager/keymanager.go (left):

https://codereview.appspot.com/70190050/diff/60001/state/apiserver/keymanager...
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#ne...
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#old...
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/firewall...
File worker/firewaller/firewaller_test.go (left):

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

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

https://codereview.appspot.com/70190050/diff/60001/worker/instancepoller/obse...
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/machineenvironmentw...
File worker/machineenvironmentworker/machineenvironmentworker_test.go (right):

https://codereview.appspot.com/70190050/diff/60001/worker/machineenvironmentw...
worker/machineenvironmentworker/machineenvironmentworker_test.go:128: attrs :=
map[string]interface{}{}
The conventional way to write this is
   attrs := make(map[string]interface{})

[waigani, 2014-03-13 01:02:02]

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#ne...
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....
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...
File state/api/firewaller/state_test.go (right):

https://codereview.appspot.com/70190050/diff/60001/state/api/firewaller/state...
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...
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/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/70190050/diff/60001/state/apiserver/client/cli...
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/cli...
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/cli...
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...
File state/apiserver/keymanager/keymanager.go (left):

https://codereview.appspot.com/70190050/diff/60001/state/apiserver/keymanager...
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#ne...
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#old...
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/obse...
File worker/instancepoller/observer_test.go (left):

https://codereview.appspot.com/70190050/diff/60001/worker/instancepoller/obse...
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/machineenvironmentw...
File worker/machineenvironmentworker/machineenvironmentworker_test.go (right):

https://codereview.appspot.com/70190050/diff/60001/worker/machineenvironmentw...
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?

[waigani, 2014-03-13 01:02:21]

Yikes. I really dug a hole for myself with such a big branch. I've finally
merged in trunk, all tests pass and review comments addressed. Let me know if
I've missed anything.

[axw, 2014-03-13 03:07:13]

Getting there.

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

https://codereview.appspot.com/70190050/diff/60001/state/apiserver/keymanager...
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#old...
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...

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

https://codereview.appspot.com/70190050/diff/60001/worker/machineenvironmentw...
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....
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#ne...
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/e...
File state/api/common/testing/environwatcher.go (right):

https://codereview.appspot.com/70190050/diff/80001/state/api/common/testing/e...
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...
File state/apiserver/keymanager/keymanager.go (right):

https://codereview.appspot.com/70190050/diff/80001/state/apiserver/keymanager...
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#oldcod...
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#oldcod...
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#newcod...
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/provisi...
File worker/provisioner/provisioner_test.go (left):

https://codereview.appspot.com/70190050/diff/80001/worker/provisioner/provisi...
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/provisi...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/70190050/diff/80001/worker/provisioner/provisi...
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.

[waigani, 2014-03-17 09:31:34]

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#old...
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...

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....
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/e...
File state/api/common/testing/environwatcher.go (right):

https://codereview.appspot.com/70190050/diff/80001/state/api/common/testing/e...
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...
File state/apiserver/keymanager/keymanager.go (right):

https://codereview.appspot.com/70190050/diff/80001/state/apiserver/keymanager...
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#oldcod...
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#oldcod...
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#newcod...
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/provisi...
File worker/provisioner/provisioner_test.go (left):

https://codereview.appspot.com/70190050/diff/80001/worker/provisioner/provisi...
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/provisi...
File worker/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/70190050/diff/80001/worker/provisioner/provisi...
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.

[axw, 2014-03-17 12:15:56]

A few minor things, then it should be good to land.

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

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go#n...
environs/statepolicy.go:43: return provider.(state.ConfigValidator), nil
No need to type assert, since EnvironProvider implements state.ConfigValidator.
Just "return provider, nil".

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go#n...
environs/statepolicy.go:44: return nil,
errors.NewNotImplementedError("ConfigValidator")
Lose the second return

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

https://codereview.appspot.com/70190050/diff/100001/juju/conn_test.go#newcode143
juju/conn_test.go:143: //Use a state without a nil policy, which will allow us
to set an invalid config
Please leave a space after //, preferably end sentence with a .

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

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/...
state/api/common/testing/environwatcher.go:92: err =
s.st.UpdateEnvironConfig(map[string]interface{}{}, []string{"foo"}, nil)
check error

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/...
state/api/common/testing/environwatcher.go:94: c.Logf("!!!!! %#v",
cf.AllAttrs()["logging-config"])
leftover logging

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

https://codereview.appspot.com/70190050/diff/100001/state/state.go#newcode279
state/state.go:279: type ValidateFn func(updateAttrs map[string]interface{},
removeAttrs []string, oldConfig *config.Config) error
This could do with a better name. Out of context (e.g. in godoc), it wouldn't
really be meaningful. I suggest ValidateConfigFunc (Func, rather than Fn, to be
consistent with function types in Go's standard library).

https://codereview.appspot.com/70190050/diff/100001/state/state.go#newcode316
state/state.go:316: //use validCfg to update settings
Just drop this comment

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

https://codereview.appspot.com/70190050/diff/100001/state/state_test.go#newco...
state/state_test.go:1689: if v, found := updateAttrs["agent-version"]; found {
All we really care about is: did the callback get called with the appropriate
arguments, and does its result have the appropriate effect?

So there's some missing coverage there: there's no validation of the input, and
there's no test for a non-nil validator with a nil result (i.e. has no effect on
normal operation).

https://codereview.appspot.com/70190050/diff/100001/state/state_test.go#newco...
state/state_test.go:1818: err = settings.Update(nil, bson.D{{"$set",
bson.D{{"name", "foo"}}}})
What is this doing here? Is this because UpdateEnvironConfig won't change the
"name" attribute? If it's needed, you should probably be using
settings.UpdateId.

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

https://codereview.appspot.com/70190050/diff/100001/worker/provisioner/provis...
worker/provisioner/provisioner_test.go:595: func (s *ProvisionerSuite)
TestProvisioningRecoversAfterInvalidEnvironmentPublished(c *gc.C) {
As before, reinstate until we can guarantee config validity.

[waigani, 2014-03-17 22:45:52]

Please take a look.

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

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go#n...
environs/statepolicy.go:43: return provider.(state.ConfigValidator), nil
On 2014/03/17 12:15:56, axw wrote:
> No need to type assert, since EnvironProvider implements
state.ConfigValidator.
> Just "return provider, nil".

Done.

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go#n...
environs/statepolicy.go:44: return nil,
errors.NewNotImplementedError("ConfigValidator")
On 2014/03/17 12:15:56, axw wrote:
> Lose the second return

Done.

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

https://codereview.appspot.com/70190050/diff/100001/juju/conn_test.go#newcode143
juju/conn_test.go:143: //Use a state without a nil policy, which will allow us
to set an invalid config
On 2014/03/17 12:15:56, axw wrote:
> Please leave a space after //, preferably end sentence with a .

Done.

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

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/...
state/api/common/testing/environwatcher.go:92: err =
s.st.UpdateEnvironConfig(map[string]interface{}{}, []string{"foo"}, nil)
On 2014/03/17 12:15:56, axw wrote:
> check error

Done.

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/...
state/api/common/testing/environwatcher.go:94: c.Logf("!!!!! %#v",
cf.AllAttrs()["logging-config"])
On 2014/03/17 12:15:56, axw wrote:
> leftover logging

Done.

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

https://codereview.appspot.com/70190050/diff/100001/state/state.go#newcode279
state/state.go:279: type ValidateFn func(updateAttrs map[string]interface{},
removeAttrs []string, oldConfig *config.Config) error
On 2014/03/17 12:15:56, axw wrote:
> This could do with a better name. Out of context (e.g. in godoc), it wouldn't
> really be meaningful. I suggest ValidateConfigFunc (Func, rather than Fn, to
be
> consistent with function types in Go's standard library).

Done.

https://codereview.appspot.com/70190050/diff/100001/state/state.go#newcode316
state/state.go:316: //use validCfg to update settings
On 2014/03/17 12:15:56, axw wrote:
> Just drop this comment

Done.

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

https://codereview.appspot.com/70190050/diff/100001/state/state_test.go#newco...
state/state_test.go:1689: if v, found := updateAttrs["agent-version"]; found {
On 2014/03/17 12:15:56, axw wrote:
> All we really care about is: did the callback get called with the appropriate
> arguments, and does its result have the appropriate effect?
> 
> So there's some missing coverage there: there's no validation of the input,
and
> there's no test for a non-nil validator with a nil result (i.e. has no effect
on
> normal operation).

Done.

https://codereview.appspot.com/70190050/diff/100001/state/state_test.go#newco...
state/state_test.go:1818: err = settings.Update(nil, bson.D{{"$set",
bson.D{{"name", "foo"}}}})
On 2014/03/17 12:15:56, axw wrote:
> What is this doing here? Is this because UpdateEnvironConfig won't change the
> "name" attribute? If it's needed, you should probably be using
> settings.UpdateId.

Done. Changed to UpdateId. As "name" has been removed, settings errors out in
UpdateEnvironConig. I'm not sure if this is the best way around it, but it is a
way.

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

https://codereview.appspot.com/70190050/diff/100001/worker/provisioner/provis...
worker/provisioner/provisioner_test.go:595: func (s *ProvisionerSuite)
TestProvisioningRecoversAfterInvalidEnvironmentPublished(c *gc.C) {
On 2014/03/17 12:15:56, axw wrote:
> As before, reinstate until we can guarantee config validity.

Done.

[axw, 2014-03-18 02:23:41]

On 2014/03/17 22:48:37, waigani wrote:

LGTM. Thanks for slogging through it.