map-backed Config type

map-backed Config type

The bulk of this diff is:

  * code moves -- breaking up the morass that is the dummy env, moving
    misplaced stuff like authorized-keys handling out of ec2, generally
	trying to put stuff in places that make sense
  * name changes -- ReadEnvironsBytes->NewConfigSet, etc
  * somewhat detailed tests for config handling and updating

...so it's not quite as big as it looks.

But... it would appear that I Just Don't Get It :(. All this mucking around
in maps seems to me to be profoundly ugly and *very* hard to get right
(hence the large amount of seriously pedantic testing).

I was led towards implementing config.ComposeConfig and config.Checker
because of the otherwise *hideous* duplication between the dummy and
ec2 environs; and the fact that *all* the tests in config would have to be
dumped into ec2 and duplicated in all future providers.

If you don't like this, and I wouldn't blame you if you didn't, I'm not
sure what else I can do. I really tried...

(Incidentally... I just don't know whether I'm following sensible practices
here. If I don't tidy code as I go I get nice clean diffs and we end up with
a tangled morass of code after a few weeks (witness state tests, dummy
environs, etc); if I do tidy as I go we get tangled diffs but a chance at
gradually improving code consistency. Anyone?)

https://code.launchpad.net/~fwereade/juju-core/env-config-trainwreck/+merge/114654

(do not edit description out of merge proposal)

[fwereade, 2012-07-12 15:25:14]

Please take a look.

[rog, 2012-07-12 17:31:04]

Generally I think this looks pretty good. The stuff in ec2 could be done more
easily, I think - we'll talk online.

https://codereview.appspot.com/6353092/diff/2001/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6353092/diff/2001/cmd/juju/cmd_test.go#newcode165
cmd/juju/cmd_test.go:165: cs, err := environs.ReadConfigSet("")
could we spell ConfigSet "Environs", and thereby keep the interface the same as
before?

https://codereview.appspot.com/6353092/diff/2001/environs/dummy/provider.go
File environs/dummy/provider.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/dummy/provider.go#n...
environs/dummy/provider.go:84: }
i'm not sure i see why any of the above verification is necessary. why shouldn't
the base config have a missing zookeeper? why shouldn't we allow the zookeeper
entry to change?

https://codereview.appspot.com/6353092/diff/2001/environs/dummy/provider.go#n...
environs/dummy/provider.go:90: }
same applies here

https://codereview.appspot.com/6353092/diff/2001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/ec2/ec2.go#newcode82
environs/ec2/ec2.go:82: // mu protects all other fields below.
the original "Unlocked" names were chosen deliberately. changing them in this CL
seems like unnecessary churn in an already-large CL.

https://codereview.appspot.com/6353092/diff/2001/environs/ec2/provider_test.go
File environs/ec2/provider_test.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/ec2/provider_test.g...
environs/ec2/provider_test.go:1: package ec2_test
this is all still testing the configuration. why not config_test.go ?

https://codereview.appspot.com/6353092/diff/2001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/interface.go#newcode92
environs/interface.go:92: SetConfig(*config.Config) error
if these method definitions were left in the same place, there would be less
churn.

https://codereview.appspot.com/6353092/diff/2001/environs/provider.go
File environs/provider.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/provider.go#newcode9
environs/provider.go:9: type Provider interface {
i'd prefer it if this remained in interface.go - that way all the required
environs interface is easily seen in one file.

https://codereview.appspot.com/6353092/diff/2001/schema/schema.go
File schema/schema.go (right):

https://codereview.appspot.com/6353092/diff/2001/schema/schema.go#newcode167
schema/schema.go:167: func ContentString() Checker {
NonEmptyString?

[fwereade, 2012-07-12 22:53:33]

Thanks, that's reassuring :). Haven't fixed any of these yet, but I have a bit
of time to talk tomorrow; don't need to be off until the afternoon. Very keen to
hear your ideas about the EC2 provider's ComposeConfig...

https://codereview.appspot.com/6353092/diff/2001/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6353092/diff/2001/cmd/juju/cmd_test.go#newcode165
cmd/juju/cmd_test.go:165: cs, err := environs.ReadConfigSet("")
On 2012/07/12 17:31:04, rog wrote:
> could we spell ConfigSet "Environs", and thereby keep the interface the same
as
> before?

I'm not sure... I think the blurring of lines between Environ and Config makes
things harder to understand, not easier, but I'm finding it hard to get worked
up over naming issues ATM and am happy to follow consensus. Anyone else?

https://codereview.appspot.com/6353092/diff/2001/environs/dummy/provider.go
File environs/dummy/provider.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/dummy/provider.go#n...
environs/dummy/provider.go:84: }
On 2012/07/12 17:31:04, rog wrote:
> i'm not sure i see why any of the above verification is necessary. why
shouldn't
> the base config have a missing zookeeper? why shouldn't we allow the zookeeper
> entry to change?

Primarily because I don't know the dummy environ well enough to think through
the consequences of it... seems somewhat sketchy to me, though. What happens
when someone adds a zookeeper after bootstrap? How does allowing this help us?

https://codereview.appspot.com/6353092/diff/2001/environs/dummy/provider.go#n...
environs/dummy/provider.go:90: }
On 2012/07/12 17:31:04, rog wrote:
> same applies here

Looking at usage of isBroken in isolation, I'm much more comfortable with
allowing this to change.

https://codereview.appspot.com/6353092/diff/2001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/ec2/ec2.go#newcode82
environs/ec2/ec2.go:82: // mu protects all other fields below.
On 2012/07/12 17:31:04, rog wrote:
> the original "Unlocked" names were chosen deliberately. changing them in this
CL
> seems like unnecessary churn in an already-large CL.

Heh, on encountering them they seemed to smack greatly of overthinking... I'll
change them back if you like, but I'd be interested to know what "Unlocked"
indicates that is not implicitly true about every field on every type :).

https://codereview.appspot.com/6353092/diff/2001/environs/ec2/provider_test.go
File environs/ec2/provider_test.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/ec2/provider_test.g...
environs/ec2/provider_test.go:1: package ec2_test
On 2012/07/12 17:31:04, rog wrote:
> this is all still testing the configuration. why not config_test.go ?

Because it's testing the *provider*. Yes, it's testing what the provider does
with Configs, hence the suite name, but the only testing of Config itself is
*very* light and casual, and is sufficient only to infer that, yes, we're using
the Config type and it looks like it acts how it's tested in far more detail
elsewhere.

config_test made perfect sense when we had an actual internal config type to
test, but that doesn't seem to me to apply any more.

https://codereview.appspot.com/6353092/diff/2001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/interface.go#newcode92
environs/interface.go:92: SetConfig(*config.Config) error
On 2012/07/12 17:31:04, rog wrote:
> if these method definitions were left in the same place, there would be less
> churn.

Maybe I screwed it up, but the intent was to make the method ordering consistent
across the interface definition and the various implementations. This is the
sort of thing I was talking about in the decription -- if I ignore "minor"
issues like this the code gets gradually harder to follow, but if I fix them
it's seen as churn; and if I leave them to separate CLs it just doesn't happen,
because this sort of casual inconsistency only *hurts* when I'm directly working
with the code.

https://codereview.appspot.com/6353092/diff/2001/environs/provider.go
File environs/provider.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/provider.go#newcode9
environs/provider.go:9: type Provider interface {
On 2012/07/12 17:31:04, rog wrote:
> i'd prefer it if this remained in interface.go - that way all the required
> environs interface is easily seen in one file.

I'm not sure that's relevant to anyone who isn't implementing their own provider
-- for everyone else, there's a significant distinction between the Provider
interface and the Environ/Instance interfaces, because "normal" users don't have
to care about Provider itself at all; and New and ComposeConfig certainly don't
belong in interface.go, but they do belong "with" Provider more than they do
anything else, I think. Crazy?

https://codereview.appspot.com/6353092/diff/2001/schema/schema.go
File schema/schema.go (right):

https://codereview.appspot.com/6353092/diff/2001/schema/schema.go#newcode167
schema/schema.go:167: func ContentString() Checker {
On 2012/07/12 17:31:04, rog wrote:
> NonEmptyString?

Felt kind weird when I tried it, but I think you're right all the same. Cheers.

[fwereade, 2012-07-12 23:05:29]

https://codereview.appspot.com/6353092/diff/2001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/interface.go#newcode92
environs/interface.go:92: SetConfig(*config.Config) error
On 2012/07/12 22:53:34, fwereade wrote:
> On 2012/07/12 17:31:04, rog wrote:
> > if these method definitions were left in the same place, there would be less
> > churn.
> 
> Maybe I screwed it up, but the intent was to make the method ordering
consistent
> across the interface definition and the various implementations. This is the
> sort of thing I was talking about in the decription -- if I ignore "minor"
> issues like this the code gets gradually harder to follow, but if I fix them
> it's seen as churn; and if I leave them to separate CLs it just doesn't
happen,
> because this sort of casual inconsistency only *hurts* when I'm directly
working
> with the code.

Looks like I did screw it up :-/. Would you be seriously opposed to me actually
making the ordering consistent?

[dave_cheney.net, 2012-07-13 02:18:37]

Lovely work.

https://codereview.appspot.com/6353092/diff/2001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/config/config.go#ne...
environs/config/config.go:1: package config
very nice.

https://codereview.appspot.com/6353092/diff/2001/environs/config/config_test.go
File environs/config/config_test.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/config/config_test....
environs/config/config_test.go:26: err := os.Mkdir(dotssh, 0777)
nit: .ssh is usually 0700, sshd will reject it if someone other than the owner
can modify files inside it. As this is a test, this is probably not relevant.

https://codereview.appspot.com/6353092/diff/2001/environs/config/config_test....
environs/config/config_test.go:31: err = os.Mkdir(alt, 0777)
ditto

https://codereview.appspot.com/6353092/diff/2001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/dummy/environs.go#n...
environs/dummy/environs.go:212: return e.cfg.Map()["broken"].(bool)
Do you think there is value in adding convenience methods on config for (say)
string and bool types to avoid calling Map() ?

https://codereview.appspot.com/6353092/diff/2001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/ec2/ec2.go#newcode82
environs/ec2/ec2.go:82: // mu protects all other fields below.
> change them back if you like, but I'd be interested to know what "Unlocked"
> indicates that is not implicitly true about every field on every type :).

This was at the request of Gustavo. The `Unlocked` prefix is supposed to warn
you that if you touch them without `unlocking` them first via grabbing the
mutex, you're a naughty boy.

https://codereview.appspot.com/6353092/diff/2001/environs/ec2/live_test.go
File environs/ec2/live_test.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/ec2/live_test.go#ne...
environs/ec2/live_test.go:54: Name:             "",
Blank ?

https://codereview.appspot.com/6353092/diff/2001/environs/ec2/provider.go
File environs/ec2/provider.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/ec2/provider.go#new...
environs/ec2/provider.go:75: secretKey, _ := final["secret-key"].(string)
damn I love the zero value.

https://codereview.appspot.com/6353092/diff/2001/environs/ec2/provider_test.go
File environs/ec2/provider_test.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/ec2/provider_test.g...
environs/ec2/provider_test.go:26: err := os.Mkdir(dotssh, 0777)
nit: access bits.

https://codereview.appspot.com/6353092/diff/2001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/interface.go#newcode92
environs/interface.go:92: SetConfig(*config.Config) error
LGTM.

https://codereview.appspot.com/6353092/diff/2001/environs/jujutest/test.go
File environs/jujutest/test.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/jujutest/test.go#ne...
environs/jujutest/test.go:22: Configs *environs.ConfigSet
nit: Config, or CS maybe

https://codereview.appspot.com/6353092/diff/2001/environs/jujutest/test.go#ne...
environs/jujutest/test.go:55: Configs *environs.ConfigSet
as above

https://codereview.appspot.com/6353092/diff/2001/environs/provider.go
File environs/provider.go (right):

https://codereview.appspot.com/6353092/diff/2001/environs/provider.go#newcode9
environs/provider.go:9: type Provider interface {
I don't think this ads much value to move it around, everything is in one pkg
from godoc's point of view.

https://codereview.appspot.com/6353092/diff/2001/service/provisioner/provisio...
File service/provisioner/provisioner.go (right):

https://codereview.appspot.com/6353092/diff/2001/service/provisioner/provisio...
service/provisioner/provisioner.go:100: newcfg, err :=
environs.ComposeConfig(cfg, node.Map())
I do like the name ComposeConfig.

[fwereade, 2012-07-13 16:41:54]

On 2012/07/13 02:18:37, dfc wrote:
> Lovely work.
> 
> https://codereview.appspot.com/6353092/diff/2001/environs/config/config.go
> File environs/config/config.go (right):
> 
>
https://codereview.appspot.com/6353092/diff/2001/environs/config/config.go#ne...
> environs/config/config.go:1: package config
> very nice.
> 
>
https://codereview.appspot.com/6353092/diff/2001/environs/config/config_test.go
> File environs/config/config_test.go (right):
> 
>
https://codereview.appspot.com/6353092/diff/2001/environs/config/config_test....
> environs/config/config_test.go:26: err := os.Mkdir(dotssh, 0777)
> nit: .ssh is usually 0700, sshd will reject it if someone other than the owner
> can modify files inside it. As this is a test, this is probably not relevant.
> 
>
https://codereview.appspot.com/6353092/diff/2001/environs/config/config_test....
> environs/config/config_test.go:31: err = os.Mkdir(alt, 0777)
> ditto
> 
> https://codereview.appspot.com/6353092/diff/2001/environs/dummy/environs.go
> File environs/dummy/environs.go (right):
> 
>
https://codereview.appspot.com/6353092/diff/2001/environs/dummy/environs.go#n...
> environs/dummy/environs.go:212: return e.cfg.Map()["broken"].(bool)
> Do you think there is value in adding convenience methods on config for (say)
> string and bool types to avoid calling Map() ?
> 
> https://codereview.appspot.com/6353092/diff/2001/environs/ec2/ec2.go
> File environs/ec2/ec2.go (right):
> 
> https://codereview.appspot.com/6353092/diff/2001/environs/ec2/ec2.go#newcode82
> environs/ec2/ec2.go:82: // mu protects all other fields below.
> > change them back if you like, but I'd be interested to know what "Unlocked"
> > indicates that is not implicitly true about every field on every type :).
> 
> This was at the request of Gustavo. The `Unlocked` prefix is supposed to warn
> you that if you touch them without `unlocking` them first via grabbing the
> mutex, you're a naughty boy.
> 
> https://codereview.appspot.com/6353092/diff/2001/environs/ec2/live_test.go
> File environs/ec2/live_test.go (right):
> 
>
https://codereview.appspot.com/6353092/diff/2001/environs/ec2/live_test.go#ne...
> environs/ec2/live_test.go:54: Name:             "",
> Blank ?
> 
> https://codereview.appspot.com/6353092/diff/2001/environs/ec2/provider.go
> File environs/ec2/provider.go (right):
> 
>
https://codereview.appspot.com/6353092/diff/2001/environs/ec2/provider.go#new...
> environs/ec2/provider.go:75: secretKey, _ := final["secret-key"].(string)
> damn I love the zero value.
> 
> https://codereview.appspot.com/6353092/diff/2001/environs/ec2/provider_test.go
> File environs/ec2/provider_test.go (right):
> 
>
https://codereview.appspot.com/6353092/diff/2001/environs/ec2/provider_test.g...
> environs/ec2/provider_test.go:26: err := os.Mkdir(dotssh, 0777)
> nit: access bits.
> 
> https://codereview.appspot.com/6353092/diff/2001/environs/interface.go
> File environs/interface.go (right):
> 
>
https://codereview.appspot.com/6353092/diff/2001/environs/interface.go#newcode92
> environs/interface.go:92: SetConfig(*config.Config) error
> LGTM.
> 
> https://codereview.appspot.com/6353092/diff/2001/environs/jujutest/test.go
> File environs/jujutest/test.go (right):
> 
>
https://codereview.appspot.com/6353092/diff/2001/environs/jujutest/test.go#ne...
> environs/jujutest/test.go:22: Configs *environs.ConfigSet
> nit: Config, or CS maybe
> 
>
https://codereview.appspot.com/6353092/diff/2001/environs/jujutest/test.go#ne...
> environs/jujutest/test.go:55: Configs *environs.ConfigSet
> as above
> 
> https://codereview.appspot.com/6353092/diff/2001/environs/provider.go
> File environs/provider.go (right):
> 
> https://codereview.appspot.com/6353092/diff/2001/environs/provider.go#newcode9
> environs/provider.go:9: type Provider interface {
> I don't think this ads much value to move it around, everything is in one pkg
> from godoc's point of view.
> 
>
https://codereview.appspot.com/6353092/diff/2001/service/provisioner/provisio...
> File service/provisioner/provisioner.go (right):
> 
>
https://codereview.appspot.com/6353092/diff/2001/service/provisioner/provisio...
> service/provisioner/provisioner.go:100: newcfg, err :=
> environs.ComposeConfig(cfg, node.Map())
> I do like the name ComposeConfig.

Dave, thanks very much for the review, but rog has a far nicer approach IMO so
I'm dropping this. Sorry to waste your time :(.