environs: add NewEnviron(kind, config)

environs: add NewEnviron(kind, config)

NewEnviron is used to convert the a *ConfigNode (retrieved 
from state.WatchEnvironment) into a functioning Environ.

https://code.launchpad.net/~dave-cheney/juju/go-environ-schema/+merge/105769

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-05-15 07:58:46]

Please take a look.

[TheMue, 2012-05-15 08:06:32]

Mostly LGTM. Understand your feelings regarding the usage of interface{}. Also
have one question about Check().

https://codereview.appspot.com/6215045/diff/1001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6215045/diff/1001/environs/interface.go#newcode11
environs/interface.go:11: // Check is used to validate the configuration
attributes.
If Check() only checks, what is the returned interface{}? At least named return
values could help here.

[dave_cheney.net, 2012-05-15 08:12:22]

Please take a look.

[rog, 2012-05-15 08:59:29]

not sure about this. misgivings below.

https://codereview.appspot.com/6215045/diff/13/environs/config.go
File environs/config.go (left):

https://codereview.appspot.com/6215045/diff/13/environs/config.go#oldcode15
environs/config.go:15: config interface{} // the configuration data for passing
to NewEnviron.
can't believe i never saw those typos!

https://codereview.appspot.com/6215045/diff/13/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/6215045/diff/13/environs/open.go#newcode36
environs/open.go:36: cfg, err := p.Check(config)
If Environs.Check must always accept a map[string]interface{}, then the method
signature should specify that. However, my original idea, which I still think
could work ok, was to have Check take a Getter:

type Getter interface {
    Get(key string) (value interface{}, found bool)
}

then we can pass a *ConfigNode directly to NewEnviron and thence to p.Check.

To save config settings, we'd define a Setter interface:

type Setter interface {
    Set(key string, value interface{})
}

and define a new method (Environ.Write? Environ.Save?) which would use a Setter
to write out all its values.

[niemeyer, 2012-05-15 21:03:13]

https://codereview.appspot.com/6215045/diff/13/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6215045/diff/13/environs/dummy/environs.go#new...
environs/dummy/environs.go:122: func (e *environProvider) Check(attributes
interface{}) (interface{}, error) {
I'm not following the purpose of the refactoring here. Note that it's not
actually just checking. It is indeed coercing the values, which is more clearly
spelled out in the previous version.

https://codereview.appspot.com/6215045/diff/13/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6215045/diff/13/environs/interface.go#newcode16
environs/interface.go:16: // Open creates a new Environ with the attributes
returned by Check.
The function name change should go in, independent of the outcome of this CL,
though.

https://codereview.appspot.com/6215045/diff/13/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/6215045/diff/13/environs/open.go#newcode36
environs/open.go:36: cfg, err := p.Check(config)
On 2012/05/15 08:59:30, rog wrote:
> If Environs.Check must always accept a map[string]interface{}, then the method
> signature should specify that. However, my original idea, which I still think
> could work ok, was to have Check take a Getter:

I like part of your idea, but this specific bit doesn't seem to fit well with
the rest of it. The blob that goes through the coercion is an arbitrarily typed
map. It feels strange to have *that* map within a Getter. I suggest we keep the
logic of the coercion in interface with ConfigChecker, and have a function like
this within environs:

func CoerceConfig(e EnvironProvider, attrs interface{}) (config Getter, err
error)

This may be used both when parsing environments.yaml, and when parsing the
/environment node within ZooKeeper.

The EnvironProvider then becomes:

    func ConfigChecker() schema.Checker
    func Open(config Getter) (Environ, error)

> and define a new method (Environ.Write? Environ.Save?) which would use a
Setter
> to write out all its values.

+1 on Save. Write is generally associated with []byte or io.Writer.

[dave_cheney.net, 2012-05-15 23:16:01]

Please take a look.

https://codereview.appspot.com/6215045/diff/13/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6215045/diff/13/environs/dummy/environs.go#new...
environs/dummy/environs.go:122: func (e *environProvider) Check(attributes
interface{}) (interface{}, error) {
I'll revert this and see what it looks like. One observation, you can't use any
attributes without coercing them through a ConfigChecker, so it's not really
checking, but translation.

On 2012/05/15 21:03:14, niemeyer wrote:
> I'm not following the purpose of the refactoring here. Note that it's not
> actually just checking. It is indeed coercing the values, which is more
clearly
> spelled out in the previous version.

https://codereview.appspot.com/6215045/diff/13/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6215045/diff/13/environs/interface.go#newcode16
environs/interface.go:16: // Open creates a new Environ with the attributes
returned by Check.
On 2012/05/15 21:03:14, niemeyer wrote:
> The function name change should go in, independent of the outcome of this CL,
> though.

Done.

https://codereview.appspot.com/6215045/diff/13/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/6215045/diff/13/environs/open.go#newcode36
environs/open.go:36: cfg, err := p.Check(config)
> then we can pass a *ConfigNode directly to NewEnviron and thence to p.Check.

It won't work, environProvider.Check() expects a map. It's not a big problem to
call .Map on the *ConfigNode.
 
> To save config settings, we'd define a Setter interface:
> 
> type Setter interface {
>     Set(key string, value interface{})
> }
> 
> and define a new method (Environ.Write? Environ.Save?) which would use a
Setter
> to write out all its values.

[dave_cheney.net, 2012-05-15 23:22:16]

Please take a look.

[rog, 2012-05-16 00:30:32]

On May 15, 2012 10:03 PM, <n13m3y3r@gmail.com> wrote:
>
>
> https://codereview.appspot.com/6215045/diff/13/environs/dummy/environs.go
> File environs/dummy/environs.go (right):
>
>
https://codereview.appspot.com/6215045/diff/13/environs/dummy/environs.go#new...
> environs/dummy/environs.go:122: func (e *environProvider)
>
> Check(attributes interface{}) (interface{}, error) {
> I'm not following the purpose of the refactoring here. Note that it's
> not actually just checking. It is indeed coercing the values, which is
> more clearly spelled out in the previous version.
>
> https://codereview.appspot.com/6215045/diff/13/environs/interface.go
> File environs/interface.go (right):
>
>
https://codereview.appspot.com/6215045/diff/13/environs/interface.go#newcode16
> environs/interface.go:16: // Open creates a new Environ with the
> attributes returned by Check.
> The function name change should go in, independent of the outcome of
> this CL, though.
>
>
> https://codereview.appspot.com/6215045/diff/13/environs/open.go
> File environs/open.go (right):
>
> https://codereview.appspot.com/6215045/diff/13/environs/open.go#newcode36
> environs/open.go:36: cfg, err := p.Check(config)
> On 2012/05/15 08:59:30, rog wrote:
>>
>> If Environs.Check must always accept a map[string]interface{}, then
>
> the method
>>
>> signature should specify that. However, my original idea, which I
>
> still think
>>
>> could work ok, was to have Check take a Getter:
>
>
> I like part of your idea, but this specific bit doesn't seem to fit well
> with the rest of it. The blob that goes through the coercion is an
> arbitrarily typed map. It feels strange to have *that* map within a
> Getter. I suggest we keep the logic of the coercion in interface with
> ConfigChecker, and have a function like this within environs:

It's late at night and I've been at the pub so I may be misguided, but I
think that the whole point is that we are proposing that the thing that
goes into the coercion fn be *not* an arbitrarily typed map, but a
structured map with string keys. That way it can potentially coexist with
other keys at the same level. The thing that comes *out* of Coerce is
arbitrarily typed - whatever the Environ would like
to pass to itself.

I'm probably wrong!

> func CoerceConfig(e EnvironProvider, attrs interface{}) (config Getter,
> err error)
>
> This may be used both when parsing environments.yaml, and when parsing
> the /environment node within ZooKeeper.
>
> The EnvironProvider then becomes:
>
>    func ConfigChecker() schema.Checker
>    func Open(config Getter) (Environ, error)
>
>
>> and define a new method (Environ.Write? Environ.Save?) which would use
>
> a Setter
>>
>> to write out all its values.
>
>
> +1 on Save. Write is generally associated with []byte or io.Writer.
>
> https://codereview.appspot.com/6215045/

[gustavo_niemeyer.net, 2012-05-16 02:11:54]

On Tue, May 15, 2012 at 9:30 PM, roger peppe <rogpeppe@gmail.com> wrote:
> It's late at night and I've been at the pub so I may be misguided, but I
> think that the whole point is that we are proposing that the thing that goes
> into the coercion fn be *not* an arbitrarily typed map, but a structured map

Is that the whole point? Why?

> with string keys. That way it can potentially coexist with other keys at the
> same level. The thing that comes *out* of Coerce is arbitrarily typed -
> whatever the Environ would like to pass to itself.

The thing going into Coerce is a loose unchecked and perhaps mistyped
or broken bag of data. The thing that comes out of Coerce is properly
typed, and validated.  It may be useful to have a more structured
interface on the latter, but a map works fine for the former, IMO. If
there's still disagreement, let's try to surface the use cases.

> I'm probably wrong!

The point isn't whether you're right or wrong.. the point is what are
we trying to do here. We're all right *and* wrong if we push in
different directions.


gustavo @ http://niemeyer.net

[rog, 2012-05-16 17:14:11]

On 2012/05/16 02:11:54, gustavo_niemeyer.net wrote:
> On Tue, May 15, 2012 at 9:30 PM, roger peppe <mailto:rogpeppe@gmail.com>
wrote:
> > It's late at night and I've been at the pub so I may be misguided, but I
> > think that the whole point is that we are proposing that the thing that goes
> > into the coercion fn be *not* an arbitrarily typed map, but a structured map
> 
> Is that the whole point? Why?

ah, i thought you were party to the conversation that ended
up with this idea. perhaps you'd already left UDS by that point.

we want to be able to store environment settings inside the state.
there are a few ways we could do this:

1) YAML inside YAML. i.e. ask an environment to marshal itself to []byte, and
put that as a string inside the state.
2) a nested YAML value. i.e. ask an environment to return an interface{} that
can be marshalled as part of the state, inside an "environ" attribute, for
example.
3) alongside the other environment settings, as regular attributes on the
top-level environment settings node.

in general i think that 2) is a good compromise, but we had decided that 3) was
desirable.
by making Environ.Check operate directly on a map[string]interface{}, we make
that possible. (as a consequence of doing so, we need to make sure *outside*
Check that we actually have a map[string]interface{} rather than an arbitrarily
typed bundle of stuff)

[niemeyer, 2012-05-16 17:18:52]

<rogpeppe> niemeyer: just replied to https://codereview.appspot.com/6215045/ .
it would probably be good to have a chat about this tomorrow.
<niemeyer> rogpeppe: Sure
<niemeyer> rogpeppe: There's a leap in your description that I can't follow
<niemeyer> rogpeppe: "by making Environ.Check operate directly on a
map[string]interface{}, we make that possible."
<niemeyer> rogpeppe: Nothing has to change in the way CheckConfig works to make
that possible

[dave_cheney.net, 2012-05-17 00:09:49]

Hello,

Is there more to discuss on this proposal?

Dave

[niemeyer, 2012-05-17 01:23:07]

This can go in, Dave. What you have here is a nice and clear step forward that
everybody seems to agree on. We can do other changes incrementally if necessary.

https://codereview.appspot.com/6215045/diff/12001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6215045/diff/12001/environs/interface.go#newco...
environs/interface.go:18: // schema.Checker.Coerce(). The name is that given in
environments.yaml.
There's a relationship between ConfigChecker and Open that was lost in the
rewrite. I suggest something like this:

// Open opens an environment with config, which must
// have been processed and validated by the schema
// checker returned by ConfigChecker.

Then, s/attributes/config/ below.

https://codereview.appspot.com/6215045/diff/12001/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/6215045/diff/12001/environs/open.go#newcode31
environs/open.go:31: func NewEnviron(kind string, config map[string]interface{})
(Environ, error) {
Looks good.

https://codereview.appspot.com/6215045/diff/12001/environs/open.go#newcode38
environs/open.go:38: return nil, fmt.Errorf("error validating environment: %v",
err)
Would this be more clear as "environment configuration error: %v"?

https://codereview.appspot.com/6215045/diff/12001/environs/open.go#newcode40
environs/open.go:40: // TODO(dfc) remove the requirement for an environment to
know it's name, it's
I think we'll continue to need it. It's used to tag machines in the environment
(i.e. on EC2) in a way that is understandable by the user when he sees the
machines listed elsewhere. In that sense, the environment has to know that its
name is "testing" for example.

I would personally find it ok if we injected the "name" attribute on the
environment configuration when reading it out of environments.yaml.

[niemeyer, 2012-05-17 01:23:21]

LGTM with the changes described.

[dave_cheney.net, 2012-05-17 01:46:52]

Please take a look.

https://codereview.appspot.com/6215045/diff/12001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6215045/diff/12001/environs/interface.go#newco...
environs/interface.go:18: // schema.Checker.Coerce(). The name is that given in
environments.yaml.
On 2012/05/17 01:23:08, niemeyer wrote:
> There's a relationship between ConfigChecker and Open that was lost in the
> rewrite. I suggest something like this:
> 
> // Open opens an environment with config, which must
> // have been processed and validated by the schema
> // checker returned by ConfigChecker.
> 
> Then, s/attributes/config/ below.

Done.

https://codereview.appspot.com/6215045/diff/12001/environs/open.go
File environs/open.go (right):

https://codereview.appspot.com/6215045/diff/12001/environs/open.go#newcode38
environs/open.go:38: return nil, fmt.Errorf("error validating environment: %v",
err)
On 2012/05/17 01:23:08, niemeyer wrote:
> Would this be more clear as "environment configuration error: %v"?

Done.

https://codereview.appspot.com/6215045/diff/12001/environs/open.go#newcode40
environs/open.go:40: // TODO(dfc) remove the requirement for an environment to
know it's name, it's
I've updated the comment with an appropriate TODO

[dave_cheney.net, 2012-05-17 01:49:45]

*** Submitted:

environs: add NewEnviron(kind, config)

NewEnviron is used to convert the a *ConfigNode (retrieved 
from state.WatchEnvironment) into a functioning Environ.

R=TheMue, rog, niemeyer, gustavo@niemeyer.net
CC=
https://codereview.appspot.com/6215045