environs: simplify EnvironProvider interface.

environs: simplify EnvironProvider interface.

We make it type safe and avoid the need to expose schema
in the interface.

https://code.launchpad.net/~rogpeppe/juju/trunk/+merge/106124

(do not edit description out of merge proposal)

[rog, 2012-05-17 09:05:14]

Please take a look.

[fwereade, 2012-05-17 09:56:18]

This LGTM but I have a vague rumination: there are a few invariably necessary
fields (name, type, default-series (for all that it's not yet implemented);
perhaps more?) and a whole bunch of provider-specific ones. Part of me leans
towards separating the common from the specific in some way... but I don't think
it's quite justifiable yet (with just one real provider and one dummy provider).

I think I was starting to feel this as a subtle pain point in the python, but a
lot of that is probably also down to the incredibly casual config handling. Er,
maybe bear it in mind for the future?

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

https://codereview.appspot.com/6213050/diff/2001/environs/config.go#newcode56
environs/config.go:56: Environments map[string]map[string]interface{}
"environments"
Heh, I hadn't appreciated how nicely map declarations "nest".

[dave_cheney.net, 2012-05-18 06:54:30]

I like it. This is approaching the designed we discussed at UDS. I like that
provider.Check() now does the checking, rather than returning something else,
which does the check.

[dave_cheney.net, 2012-05-18 06:54:51]

https://codereview.appspot.com/6213050/diff/2001/environs/config.go
File environs/config.go (left):

https://codereview.appspot.com/6213050/diff/2001/environs/config.go#oldcode14
environs/config.go:14: kind   string      // the type of environment (e.g. ec2).
Good riddance, this should be in the type implementing Environ.

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

https://codereview.appspot.com/6213050/diff/2001/environs/config.go#newcode79
environs/config.go:79: kind, _ := attrs["type"].(string)
you should put a type check here, dont' assume attrs["type"] is a string.

https://codereview.appspot.com/6213050/diff/2001/environs/config.go#newcode93
environs/config.go:93: attrs["name"] = name
Maybe a comment here

// store the name of the this environment in the config itself so that it can be
persisted into the environment config. See environs.NewEnviron

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

https://codereview.appspot.com/6213050/diff/2001/environs/dummy/environs.go#n...
environs/dummy/environs.go:142: x, err := checker.Coerce(attrs, nil)
cfg, err := ...

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

https://codereview.appspot.com/6213050/diff/2001/environs/ec2/config_test.go#...
environs/ec2/config_test.go:28: name:   "testenv",
can we have a test to ensure name is properly injected

[rog, 2012-05-21 14:11:54]

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

https://codereview.appspot.com/6213050/diff/2001/environs/config.go#newcode79
environs/config.go:79: kind, _ := attrs["type"].(string)
On 2012/05/18 06:54:51, dfc wrote:
> you should put a type check here, dont' assume attrs["type"] is a string.

i don't assume that (note the ", _"). if they provide an int for the type, i
don't mind if the error is as below. YMMV.

[niemeyer, 2012-05-21 14:55:51]

Very nice indeed. LGTM, assuming you're happy with the following suggestions:

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

https://codereview.appspot.com/6213050/diff/2001/environs/config.go#newcode56
environs/config.go:56: Environments map[string]map[string]interface{}
"environments"
Was already like that before, but the tags are unnecessary here. Conveniently,
the default value is the lowercase field name (that's the most used convention
in yaml, so makes sense). Can you please take the chance to drop them?

https://codereview.appspot.com/6213050/diff/2001/environs/config.go#newcode99
environs/config.go:99: config: cfg,
A single line would work nicely here.

https://codereview.appspot.com/6213050/diff/2001/environs/config_test.go
File environs/config_test.go (left):

https://codereview.appspot.com/6213050/diff/2001/environs/config_test.go#oldc...
environs/config_test.go:37: nil,
Not a problem in this branch (you've actually removing lines!), but this slice
could do with some reformatting. Quite ugly right now.

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

https://codereview.appspot.com/6213050/diff/2001/environs/dummy/environs.go#n...
environs/dummy/environs.go:134: "name":      schema.String(),
This should be at the top, before "type", as a convention.

https://codereview.appspot.com/6213050/diff/2001/environs/dummy/environs.go#n...
environs/dummy/environs.go:141: func (e *environProvider) Check(attrs
map[string]interface{}) (environs.EnvironConfig, error) {
s/attrs/config/ (that's what we used in the interface, right?)

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

https://codereview.appspot.com/6213050/diff/2001/environs/ec2/config.go#newco...
environs/ec2/config.go:58: v, err := configChecker.Coerce(config, nil)
Much nicer to have this here. Thanks.

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

https://codereview.appspot.com/6213050/diff/2001/environs/interface.go#newcode15
environs/interface.go:15: // The name of the environment is held in
config["name"].
This documentation is covering several things which are not related to what this
method does. I suggest being more focused, and renaming the function:

// LoadConfig returns an EnvironConfig representing the raw config map.
// Every provider must accept the "name" and "type" keys.

https://codereview.appspot.com/6213050/diff/2001/environs/interface.go#newcode20
environs/interface.go:20: // after passing through the initial Check.
On the same token:

// EnvironConfig represents an environment's configuration.

[rog, 2012-05-21 15:46:18]

Please take a look.

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

https://codereview.appspot.com/6213050/diff/2001/environs/config.go#newcode56
environs/config.go:56: Environments map[string]map[string]interface{}
"environments"
On 2012/05/21 14:55:52, niemeyer wrote:
> Was already like that before, but the tags are unnecessary here. Conveniently,
> the default value is the lowercase field name (that's the most used convention
> in yaml, so makes sense). Can you please take the chance to drop them?

Done.

https://codereview.appspot.com/6213050/diff/2001/environs/config.go#newcode93
environs/config.go:93: attrs["name"] = name
On 2012/05/18 06:54:51, dfc wrote:
> Maybe a comment here
> 
> // store the name of the this environment in the config itself so that it can
be
> persisted into the environment config. See environs.NewEnviron

Done.

https://codereview.appspot.com/6213050/diff/2001/environs/config.go#newcode99
environs/config.go:99: config: cfg,
On 2012/05/21 14:55:52, niemeyer wrote:
> A single line would work nicely here.

Done.

https://codereview.appspot.com/6213050/diff/2001/environs/config_test.go
File environs/config_test.go (left):

https://codereview.appspot.com/6213050/diff/2001/environs/config_test.go#oldc...
environs/config_test.go:37: nil,
On 2012/05/21 14:55:52, niemeyer wrote:
> Not a problem in this branch (you've actually removing lines!), but this slice
> could do with some reformatting. Quite ugly right now.

yeah, it's early code. will reformat in a later branch.

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

https://codereview.appspot.com/6213050/diff/2001/environs/dummy/environs.go#n...
environs/dummy/environs.go:134: "name":      schema.String(),
On 2012/05/21 14:55:52, niemeyer wrote:
> This should be at the top, before "type", as a convention.

Done.

https://codereview.appspot.com/6213050/diff/2001/environs/dummy/environs.go#n...
environs/dummy/environs.go:141: func (e *environProvider) Check(attrs
map[string]interface{}) (environs.EnvironConfig, error) {
On 2012/05/21 14:55:52, niemeyer wrote:
> s/attrs/config/ (that's what we used in the interface, right?)

i think attrs makes more sense here, given the type it's returning. i've changed
the name in the interface to match.

https://codereview.appspot.com/6213050/diff/2001/environs/dummy/environs.go#n...
environs/dummy/environs.go:142: x, err := checker.Coerce(attrs, nil)
On 2012/05/18 06:54:51, dfc wrote:
> cfg, err := ...

done a little different renaming. it's still awkward though. better suggestions
welcome.

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

https://codereview.appspot.com/6213050/diff/2001/environs/ec2/config_test.go#...
environs/ec2/config_test.go:28: name:   "testenv",
On 2012/05/18 06:54:51, dfc wrote:
> can we have a test to ensure name is properly injected 

not sure what you mean by that. isn't that precisely what this *is* testing?
("testenv" only gets into the config result from being injected from the
"testenv" in makeEnv)

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

https://codereview.appspot.com/6213050/diff/2001/environs/interface.go#newcode15
environs/interface.go:15: // The name of the environment is held in
config["name"].
On 2012/05/21 14:55:52, niemeyer wrote:
> This documentation is covering several things which are not related to what
this
> method does. I suggest being more focused, and renaming the function:
> 
> // LoadConfig returns an EnvironConfig representing the raw config map.
> // Every provider must accept the "name" and "type" keys.

done, but with some modifications. i prefer NewConfig to LoadConfig.

https://codereview.appspot.com/6213050/diff/2001/environs/interface.go#newcode20
environs/interface.go:20: // after passing through the initial Check.
On 2012/05/21 14:55:52, niemeyer wrote:
> On the same token:
> 
> // EnvironConfig represents an environment's configuration.

Done.

[niemeyer, 2012-05-21 16:14:01]

LGTM with a trivial.

I suggest avoiding "giving" and "gives" in comments, by the way. By now I'm
convinced the term doesn't help.

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

https://codereview.appspot.com/6213050/diff/2001/environs/config.go#newcode99
environs/config.go:99: config: cfg,
On 2012/05/21 15:46:18, rog wrote:
> On 2012/05/21 14:55:52, niemeyer wrote:
> > A single line would work nicely here.
> 
> Done.

Not a big deal, but Not Done.

https://codereview.appspot.com/6213050/diff/13003/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6213050/diff/13003/environs/interface.go#newco...
environs/interface.go:13: // accept the "name" and "type" keys, giving the name
of the
// accept the "name" and "type" attributes with the
// environment name and provider type, respectively.

Otherwise, looks good.

[rog, 2012-05-21 16:40:35]

*** Submitted:

environs: simplify EnvironProvider interface.

We make it type safe and avoid the need to expose schema
in the interface.

R=fwereade, dfc, niemeyer
CC=
https://codereview.appspot.com/6213050

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

https://codereview.appspot.com/6213050/diff/2001/environs/config.go#newcode99
environs/config.go:99: config: cfg,
On 2012/05/21 16:14:02, niemeyer wrote:
> On 2012/05/21 15:46:18, rog wrote:
> > On 2012/05/21 14:55:52, niemeyer wrote:
> > > A single line would work nicely here.
> > 
> > Done.
> 
> Not a big deal, but Not Done.

hah! done but i forgot to write the file...

https://codereview.appspot.com/6213050/diff/13003/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6213050/diff/13003/environs/interface.go#newco...
environs/interface.go:13: // accept the "name" and "type" keys, giving the name
of the
On 2012/05/21 16:14:02, niemeyer wrote:
> // accept the "name" and "type" attributes with the
> // environment name and provider type, respectively.
> 
> Otherwise, looks good.

using "holding" as discussed on IRC.