environs: turn environment configuration into a concrete type

environs: turn environment configuration into a concrete type



https://code.launchpad.net/~niemeyer/juju-core/config-3000/+merge/115203

(do not edit description out of merge proposal)

[niemeyer, 2012-07-17 02:36:32]

Please take a look.

[dave_cheney.net, 2012-07-17 06:05:18]

Hi Gustavo,

This looks reasonable, but I think I don't understand the subtitles of the
arguments for the three separate proposals.

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

https://codereview.appspot.com/6408049/diff/5001/environs/config/config_test....
environs/config/config_test.go:93: err := os.Mkdir(sshdir, 0777)
nit: ~/.ssh is be 0600, maybe the mock should follow suit

https://codereview.appspot.com/6408049/diff/5001/environs/config/current.go
File environs/config/current.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/config/current.go#n...
environs/config/current.go:28: arch = "i386"
386 is old and busted, lets just use amd64 everywhere.

[fwereade, 2012-07-17 07:19:06]

A few bits need tweaks and/or clarification, I think.

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

https://codereview.appspot.com/6408049/diff/5001/cmd/juju/cmd_test.go#newcode52
cmd/juju/cmd_test.go:52: authorized-keys: i-am-a-key
To help keep the change small (and maybe perhaps stop people from accidentally
writing tests that depend on the current series of their dev machine) I think it
would be good if the dummy provider's NewConfig was itself responsible for
inserting obvious dummy values for authorized-keys and default-series. We can
ofc set them in the config for the tests they're relevant for...

(assume similar comments for all other dummy config changes)

https://codereview.appspot.com/6408049/diff/5001/environs/config/authkeys.go
File environs/config/authkeys.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/config/authkeys.go#...
environs/config/authkeys.go:31: files = []string{"id_dsa.pub", "id_rsa.pub",
"identity.pub", "authorized_keys"}
Why do we want to allow everyone who can SSH into the host machine to SSH into
the environment? Maybe it's *convenient* but IMO it kinda violates Least
Surprise.

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go#ne...
environs/config/config.go:37: if path != "" || keys == "" {
If path *and* authorized-keys-path is empty, we should also read from
authorized-keys-path to get the defaults.

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go#ne...
environs/config/config.go:46: for _, attr := range []string{"name", "type",
"authorized-keys"} {
default-series

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go#ne...
environs/config/config.go:52: // Copy unknown attributes onto the type-specific
map.
We don't seem to use c.t except in TypeMap below; can't we just do this
as-needed?

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

https://codereview.appspot.com/6408049/diff/5001/environs/config/config_test....
environs/config/config_test.go:100: {"authorized_keys", "auth0\n#
first\nauth1\n\n"},
If we're going to have this, we should be testing that it is *not* read
automatically.

https://codereview.appspot.com/6408049/diff/5001/environs/config/current.go
File environs/config/current.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/config/current.go#n...
environs/config/current.go:28: arch = "i386"
On 2012/07/17 06:05:18, dfc wrote:
> 386 is old and busted, lets just use amd64 everywhere.

We should surely default to amd64 but I don't think there's any great benefit to
*dropping* i386 support, is there? We'll need ARM soon enough, and having 2
arches to worry about now may help keep us honest.

https://codereview.appspot.com/6408049/diff/5001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/dummy/environs.go#n...
environs/dummy/environs.go:128: broken    bool
My instinct warns me against the duplication here, but it seems to work out
rather well. Nice :).

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/config.go
File environs/ec2/config.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/config.go#newco...
environs/ec2/config.go:63: }
Out of interest, why don't we check endpoint validity here? What changes between
now and Open time?

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/config.go#newco...
environs/ec2/config.go:64: c.region = regionName
When saving the config to state, are we planning to use the maps from the
Config? If so, and if we set region on the providerConfig and not in the Config,
*and* we *ever* change the default region setting at any time in the future,
we'll kill everyone's deployments. (Well, everyone who left region unspecified,
anyway.)

(ah, yeah... *this* is why I was uncomfortable about the state duplication I
flagged in the dummy provider)

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/ec2.go#newcode116
environs/ec2/ec2.go:116: e.SetConfig(cfg)
Error checking

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/ec2.go#newcode126
environs/ec2/ec2.go:126: return fmt.Errorf("no ec2 endpoint found for region %q,
opening %q", config.region, config.name)
Shouldn't this be done immediately after picking a region when creating the
config?

Also, I may be wrong, but I don't think this is tested.

https://codereview.appspot.com/6408049/diff/5001/worker/provisioner/provision...
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/6408049/diff/5001/worker/provisioner/provision...
worker/provisioner/provisioner.go:100: p.environ.SetConfig(config)
Error checking

[niemeyer, 2012-07-17 12:10:29]

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

https://codereview.appspot.com/6408049/diff/5001/cmd/juju/cmd_test.go#newcode52
cmd/juju/cmd_test.go:52: authorized-keys: i-am-a-key
On 2012/07/17 07:19:06, fwereade wrote:
> To help keep the change small (and maybe perhaps stop people from accidentally
> writing tests that depend on the current series of their dev machine) I think
it
> would be good if the dummy provider's NewConfig was itself responsible for
> inserting obvious dummy values for authorized-keys and default-series. We can
> ofc set them in the config for the tests they're relevant for...

Yeah, this is a fuzzy area. The only purpose of the dummy provider is to be a
realistic fake environment. If we start handling it specially because it
actually doesn't *need* things to work, well.. it doesn't really need much at
all to do nothing. :-)

The question I have is: generally speaking, is authorized-keys an
across-the-board setting that all real providers should have, or is it a
per-environment setting? If it is a indeed a general setting that fits onto
*Config and is supposed to be required by all real providers, I don't feel bad
to have this being enforced on dummy too as a side-effect of us handling it in a
common location, as this CL does.

https://codereview.appspot.com/6408049/diff/5001/environs/config/authkeys.go
File environs/config/authkeys.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/config/authkeys.go#...
environs/config/authkeys.go:31: files = []string{"id_dsa.pub", "id_rsa.pub",
"identity.pub", "authorized_keys"}
On 2012/07/17 07:19:06, fwereade wrote:
> Why do we want to allow everyone who can SSH into the host machine to SSH into
> the environment? Maybe it's *convenient* but IMO it kinda violates Least
> Surprise.

Sounds good. This fix was made because the test for this disagreed with the
actual implementation, and the test was never really run as it was broken.

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go#ne...
environs/config/config.go:37: if path != "" || keys == "" {
On 2012/07/17 07:19:06, fwereade wrote:
> If path *and* authorized-keys-path is empty, we should also read from
> authorized-keys-path to get the defaults.

Indeed we do, but that seems covered already. (?)

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go#ne...
environs/config/config.go:46: for _, attr := range []string{"name", "type",
"authorized-keys"} {
On 2012/07/17 07:19:06, fwereade wrote:
> default-series

Will do.

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go#ne...
environs/config/config.go:52: // Copy unknown attributes onto the type-specific
map.
On 2012/07/17 07:19:06, fwereade wrote:
> We don't seem to use c.t except in TypeMap below; can't we just do this
> as-needed?

No, because unknowns are dropped.

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

https://codereview.appspot.com/6408049/diff/5001/environs/config/config_test....
environs/config/config_test.go:93: err := os.Mkdir(sshdir, 0777)
On 2012/07/17 06:05:18, dfc wrote:
> nit: ~/.ssh is be 0600, maybe the mock should follow suit

Agreed.

https://codereview.appspot.com/6408049/diff/5001/environs/config/config_test....
environs/config/config_test.go:100: {"authorized_keys", "auth0\n#
first\nauth1\n\n"},
On 2012/07/17 07:19:06, fwereade wrote:
> If we're going to have this, we should be testing that it is *not* read
> automatically.

Again, this test was copied from the previous location. I'm happy to drop it.

https://codereview.appspot.com/6408049/diff/5001/environs/config/current.go
File environs/config/current.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/config/current.go#n...
environs/config/current.go:28: arch = "i386"
On 2012/07/17 07:19:06, fwereade wrote:
> On 2012/07/17 06:05:18, dfc wrote:
> > 386 is old and busted, lets just use amd64 everywhere.
> 
> We should surely default to amd64 but I don't think there's any great benefit
to
> *dropping* i386 support, is there? We'll need ARM soon enough, and having 2
> arches to worry about now may help keep us honest.

This was just moved. I'm not changing it in this CL.

https://codereview.appspot.com/6408049/diff/5001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/dummy/environs.go#n...
environs/dummy/environs.go:128: broken    bool
On 2012/07/17 07:19:06, fwereade wrote:
> My instinct warns me against the duplication here, but it seems to work out
> rather well. Nice :).

I'm actually hoping to improve it further in the coming CL, but I'm trying to
get agreement on the basics first.

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/config.go
File environs/ec2/config.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/config.go#newco...
environs/ec2/config.go:64: c.region = regionName
On 2012/07/17 07:19:06, fwereade wrote:
> When saving the config to state, are we planning to use the maps from the
> Config? If so, and if we set region on the providerConfig and not in the
Config,
> *and* we *ever* change the default region setting at any time in the future,
> we'll kill everyone's deployments. (Well, everyone who left region
unspecified,
> anyway.)

This will change in Validate.


> (ah, yeah... *this* is why I was uncomfortable about the state duplication I
> flagged in the dummy provider)

I don't think I understand what you mean by that, but either way I suggest
waiting until the follow up Validate comes in.

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/ec2.go#newcode116
environs/ec2/ec2.go:116: e.SetConfig(cfg)
On 2012/07/17 07:19:06, fwereade wrote:
> Error checking

Good point, thanks.

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/ec2.go#newcode126
environs/ec2/ec2.go:126: return fmt.Errorf("no ec2 endpoint found for region %q,
opening %q", config.region, config.name)
On 2012/07/17 07:19:06, fwereade wrote:
> Shouldn't this be done immediately after picking a region when creating the
> config?
> 
> Also, I may be wrong, but I don't think this is tested.

FWIW, I don't think I dropped any tests. This code was moved from Open right
above (see the left hand side). I'm happy to introduce a new test, though.

https://codereview.appspot.com/6408049/diff/5001/worker/provisioner/provision...
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/6408049/diff/5001/worker/provisioner/provision...
worker/provisioner/provisioner.go:100: p.environ.SetConfig(config)
On 2012/07/17 07:19:06, fwereade wrote:
> Error checking

Thanks again.

[fwereade, 2012-07-17 12:46:43]

Assuming changes as discussed, LGTM to go forward with this.

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

https://codereview.appspot.com/6408049/diff/5001/cmd/juju/cmd_test.go#newcode52
cmd/juju/cmd_test.go:52: authorized-keys: i-am-a-key
On 2012/07/17 12:10:30, niemeyer wrote:
> On 2012/07/17 07:19:06, fwereade wrote:
> > To help keep the change small (and maybe perhaps stop people from
accidentally
> > writing tests that depend on the current series of their dev machine) I
think
> it
> > would be good if the dummy provider's NewConfig was itself responsible for
> > inserting obvious dummy values for authorized-keys and default-series. We
can
> > ofc set them in the config for the tests they're relevant for...
> 
> Yeah, this is a fuzzy area. The only purpose of the dummy provider is to be a
> realistic fake environment. If we start handling it specially because it
> actually doesn't *need* things to work, well.. it doesn't really need much at
> all to do nothing. :-)
> 
> The question I have is: generally speaking, is authorized-keys an
> across-the-board setting that all real providers should have, or is it a
> per-environment setting? If it is a indeed a general setting that fits onto
> *Config and is supposed to be required by all real providers, I don't feel bad
> to have this being enforced on dummy too as a side-effect of us handling it in
a
> common location, as this CL does.

Fair enough  :).

https://codereview.appspot.com/6408049/diff/5001/environs/config/authkeys.go
File environs/config/authkeys.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/config/authkeys.go#...
environs/config/authkeys.go:31: files = []string{"id_dsa.pub", "id_rsa.pub",
"identity.pub", "authorized_keys"}
On 2012/07/17 12:10:30, niemeyer wrote:
> On 2012/07/17 07:19:06, fwereade wrote:
> > Why do we want to allow everyone who can SSH into the host machine to SSH
into
> > the environment? Maybe it's *convenient* but IMO it kinda violates Least
> > Surprise.
> 
> Sounds good. This fix was made because the test for this disagreed with the
> actual implementation, and the test was never really run as it was broken.

Yeah, I had a grumpy about that. I think the test is the bit that needs fixing,
though.

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go#ne...
environs/config/config.go:37: if path != "" || keys == "" {
On 2012/07/17 12:10:30, niemeyer wrote:
> On 2012/07/17 07:19:06, fwereade wrote:
> > If path *and* authorized-keys-path is empty, we should also read from
> > authorized-keys-path to get the defaults.
> 
> Indeed we do, but that seems covered already. (?)

What happens when both path and keys are empty?

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go#ne...
environs/config/config.go:52: // Copy unknown attributes onto the type-specific
map.
On 2012/07/17 12:10:30, niemeyer wrote:
> On 2012/07/17 07:19:06, fwereade wrote:
> > We don't seem to use c.t except in TypeMap below; can't we just do this
> > as-needed?
> 
> No, because unknowns are dropped.

GAAH, sorry. Makes much more sense now.

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

https://codereview.appspot.com/6408049/diff/5001/environs/config/config_test....
environs/config/config_test.go:100: {"authorized_keys", "auth0\n#
first\nauth1\n\n"},
On 2012/07/17 12:10:30, niemeyer wrote:
> On 2012/07/17 07:19:06, fwereade wrote:
> > If we're going to have this, we should be testing that it is *not* read
> > automatically.
> 
> Again, this test was copied from the previous location. I'm happy to drop it.

+1

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/config.go
File environs/ec2/config.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/config.go#newco...
environs/ec2/config.go:64: c.region = regionName
On 2012/07/17 12:10:30, niemeyer wrote:
> On 2012/07/17 07:19:06, fwereade wrote:
> > When saving the config to state, are we planning to use the maps from the
> > Config? If so, and if we set region on the providerConfig and not in the
> Config,
> > *and* we *ever* change the default region setting at any time in the future,
> > we'll kill everyone's deployments. (Well, everyone who left region
> unspecified,
> > anyway.)
> 
> This will change in Validate.
> 
> 
> > (ah, yeah... *this* is why I was uncomfortable about the state duplication I
> > flagged in the dummy provider)
> 
> I don't think I understand what you mean by that, but either way I suggest
> waiting until the follow up Validate comes in. 

SGTM.

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/ec2.go#newcode126
environs/ec2/ec2.go:126: return fmt.Errorf("no ec2 endpoint found for region %q,
opening %q", config.region, config.name)
On 2012/07/17 12:10:30, niemeyer wrote:
> On 2012/07/17 07:19:06, fwereade wrote:
> > Shouldn't this be done immediately after picking a region when creating the
> > config?
> > 
> > Also, I may be wrong, but I don't think this is tested.
> 
> FWIW, I don't think I dropped any tests. This code was moved from Open right
> above (see the left hand side). I'm happy to introduce a new test, though.

It's a test that never existed AFAICT, no blame attaches to you ;).

But I also think it's more relevant to config than it is to open, although I
guess opinions could reasonably vary.

[rog, 2012-07-17 12:55:07]

This is looking OK. I await the Validate branch with eager anticipation :-).

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

https://codereview.appspot.com/6408049/diff/5001/cmd/juju/cmd_test.go#newcode52
cmd/juju/cmd_test.go:52: authorized-keys: i-am-a-key
On 2012/07/17 12:10:30, niemeyer wrote:
> On 2012/07/17 07:19:06, fwereade wrote:
> > To help keep the change small (and maybe perhaps stop people from
accidentally
> > writing tests that depend on the current series of their dev machine) I
think
> it
> > would be good if the dummy provider's NewConfig was itself responsible for
> > inserting obvious dummy values for authorized-keys and default-series. We
can
> > ofc set them in the config for the tests they're relevant for...
> 
> Yeah, this is a fuzzy area. The only purpose of the dummy provider is to be a
> realistic fake environment. If we start handling it specially because it
> actually doesn't *need* things to work, well.. it doesn't really need much at
> all to do nothing. :-)
> 
> The question I have is: generally speaking, is authorized-keys an
> across-the-board setting that all real providers should have, or is it a
> per-environment setting? If it is a indeed a general setting that fits onto
> *Config and is supposed to be required by all real providers, I don't feel bad
> to have this being enforced on dummy too as a side-effect of us handling it in
a
> common location, as this CL does.

i think i'm with niemeyer on this. the only awkward issue is that if someone
accidentally leaves out authorized-keys in a given test setting, the test will
likely work until we encounter a home directory with no authorized keys. for
this reason, i'm wondering if at the start of every set of tests, we set up a
dummy home directory holding the basic requirements. then we could omit
authorized-keys from this config data happy in the knowledge the the default
will be correctly inserted in all environments.

https://codereview.appspot.com/6408049/diff/5001/environs/config/authkeys.go
File environs/config/authkeys.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/config/authkeys.go#...
environs/config/authkeys.go:49: keyData = append(keyData, bytes.Trim(data,
"\n")...)
TrimRight?
though i suppose it doesn't matter much.

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go#ne...
environs/config/config.go:48: return nil, fmt.Errorf("empty %s in environment
configuration", attr)
this is a potentially misleading message - the attribute might be set, but to
something not a string.

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go#ne...
environs/config/config.go:83: func (c *Config) TypeMap() map[string]interface{}
{
i'm not keen on this name. when read in context it reads like it's something
returning a map of types.

how about Attrs and OtherAttrs?

(i think Attrs is perhaps a more accurate reflection of the returned value than
Map, and we're already using it elsewhere, e.g. environs.NewFromAttrs)

https://codereview.appspot.com/6408049/diff/5001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/dummy/environs.go#n...
environs/dummy/environs.go:291: cfg, err := st.EnvironConfig()
isn't EnvironConfig supposed to return a *config.Config?

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/config.go
File environs/ec2/config.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/config.go#newco...
environs/ec2/config.go:63: }
On 2012/07/17 07:19:06, fwereade wrote:
> Out of interest, why don't we check endpoint validity here? What changes
between
> now and Open time?

AFAIR the ec2 tests read the config data once and change the address
corresponding to the "test" region on each test.

with the advent of init-from-config, this might be something we could change.

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/config_test.go
File environs/ec2/config_test.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/config_test.go#...
environs/ec2/config_test.go:62: c.Assert(config.region, Equals, t.region)
these tests are less rigorous than it used to be, but i suppose that's ok.

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/ec2/ec2.go#newcode126
environs/ec2/ec2.go:126: return fmt.Errorf("no ec2 endpoint found for region %q,
opening %q", config.region, config.name)
On 2012/07/17 12:46:44, fwereade wrote:
> On 2012/07/17 12:10:30, niemeyer wrote:
> > On 2012/07/17 07:19:06, fwereade wrote:
> > > Shouldn't this be done immediately after picking a region when creating
the
> > > config?
> > > 
> > > Also, I may be wrong, but I don't think this is tested.
> > 
> > FWIW, I don't think I dropped any tests. This code was moved from Open right
> > above (see the left hand side). I'm happy to introduce a new test, though.
> 
> It's a test that never existed AFAICT, no blame attaches to you ;).
> 
> But I also think it's more relevant to config than it is to open, although I
> guess opinions could reasonably vary.

i made a comment about this elsewhere.

[niemeyer, 2012-07-17 13:16:25]

https://codereview.appspot.com/6408049/diff/5001/environs/config/authkeys.go
File environs/config/authkeys.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/config/authkeys.go#...
environs/config/authkeys.go:49: keyData = append(keyData, bytes.Trim(data,
"\n")...)
On 2012/07/17 12:55:07, rog wrote:
> TrimRight?

Don't see why.

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go#ne...
environs/config/config.go:48: return nil, fmt.Errorf("empty %s in environment
configuration", attr)
On 2012/07/17 12:55:07, rog wrote:
> this is a potentially misleading message - the attribute might be set, but to
> something not a string.

No, that can't happen here. The schema checker would catch it.

https://codereview.appspot.com/6408049/diff/5001/environs/config/config.go#ne...
environs/config/config.go:83: func (c *Config) TypeMap() map[string]interface{}
{
On 2012/07/17 12:55:07, rog wrote:
> i'm not keen on this name. when read in context it reads like it's something
> returning a map of types.
> 
> how about Attrs and OtherAttrs?

Doesn't sound great, but I'd be fine with AllAttrs and UnknownAttrs.

https://codereview.appspot.com/6408049/diff/5001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6408049/diff/5001/environs/dummy/environs.go#n...
environs/dummy/environs.go:291: cfg, err := st.EnvironConfig()
On 2012/07/17 12:55:07, rog wrote:
> isn't EnvironConfig supposed to return a *config.Config?

Please see the message on juju-dev regarding follow up steps.

[niemeyer, 2012-07-18 02:36:37]

*** Submitted:

environs: turn environment configuration into a concrete type

R=dfc, fwereade, rog
CC=
https://codereview.appspot.com/6408049