Remove control-bucket from environmnts.yaml

Remove control-bucket from environmnts.yaml

This is the first in a series of branches to simplify
environments.yaml by removing unnecessary attributes.
These attributes can be inferred or generated and are
stored in an .environments direcotry under JUJU_HOME,
so that they may be used again next time a command is
run.

The first attribute removed is control-bucket. The next
branches will deal with admin-secret and the certificates.
Another branch will regenerate the control-bucket if there
happens to be a name clash. We don't currently do that now,
but since the attribute is now "hidden", it is more user
firendly to do so.

https://code.launchpad.net/~wallyworld/juju-core/simple-environment-config/+merge/178003

(do not edit description out of merge proposal)

[wallyworld, 2013-08-01 05:53:08]

Please take a look.

[rog, 2013-08-02 10:51:07]

This looks like a good step forward, thanks.
Some remarks below.

https://codereview.appspot.com/12232043/diff/1/doc/glossary.txt
File doc/glossary.txt (right):

https://codereview.appspot.com/12232043/diff/1/doc/glossary.txt#newcode118
doc/glossary.txt:118: "access-key" and "secret-key" setting, but these can be
omitted if the
s/setting/settings/

https://codereview.appspot.com/12232043/diff/1/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/12232043/diff/1/environs/config.go#newcode121
environs/config.go:121: err = cfg.MergeLocalState()
I don't think it's right that ReadEnvironsBytes, which looks as if it parses a
only a slice of bytes, goes off to read extra stuff from the user's home
directory.

However, after discussion with William, I think we can leave it as is for the
time being, but in the future we can move towards a model where we just open a
single environment config, and we can then remove the entire Environs type which
is likely to be problematic in the future where we may be storing environment
configs in the cloud.

https://codereview.appspot.com/12232043/diff/1/environs/config/localstate.go
File environs/config/localstate.go (right):

https://codereview.appspot.com/12232043/diff/1/environs/config/localstate.go#...
environs/config/localstate.go:16: type LocalState struct {
I'd be tempted to do simply:

    type LocalState map[string]interface{}

Unlike Config, there's nothing interesting
to hide here.

Alternatively, we could just lose the LocalState
type entirely and just use map[string]interface{}
throughout. That would probably be my preference
actually.

https://codereview.appspot.com/12232043/diff/1/environs/config/localstate.go#...
environs/config/localstate.go:65: logger.Warningf("permission of %q is %q",
stateDir, info.Mode().Perm())

I think this is overkill and just os.MkdirAll(stateDir, 0700) would do.
if someone wants to manually chmod +w ~/.juju/.environments, they're free to do
so and i don't think a warning will help - they're unlikely to do it by
accident.

https://codereview.appspot.com/12232043/diff/1/environs/config/localstate.go#...
environs/config/localstate.go:71: if err := os.Chmod(stateFilepath, 0600); err
!= nil {


Likewise - it's quite possible that someone might *want* their environ files to
accessible to other users or groups on the same machine.

https://codereview.appspot.com/12232043/diff/1/environs/config/localstate.go#...
environs/config/localstate.go:79: // If the local state file has not yet been
created, a nil instance is returned.

I think if the local state file hasn't been created, we
should return (nil, not found error).

Returning (nil, nil) is asking for trouble IMO.

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

https://codereview.appspot.com/12232043/diff/1/environs/ec2/config.go#newcode45
environs/ec2/config.go:45: c.attrs["control-bucket"] = b

I think this side-effect from calling the controlBucket
method is a little surprising. In particular it might
come to bite us when we're trying to connect to an existing
environment that doesn't happen to have a control bucket
set or any local state - a random control bucket will be
created which corresponds to no existing environment.

I'd prefer to have an initConfig method (or something similar)
that's called explicitly in Bootstrap that fills out any
config fields that are necessary.

https://codereview.appspot.com/12232043/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/12232043/diff/1/environs/interface.go#newcode46
environs/interface.go:46: LocalAttrs(cfg *config.Config) map[string]interface{}

Why doesn't this return *config.LocalAttrs ?

https://codereview.appspot.com/12232043/diff/1/environs/openstack/config.go
File environs/openstack/config.go (right):

https://codereview.appspot.com/12232043/diff/1/environs/openstack/config.go#n...
environs/openstack/config.go:90: // control-bucket is not required in
environment.yaml anymore but we

Same concerns as for the ec2 equivalent.

[fwereade, 2013-08-02 11:19:18]

Looking very nice; I'm generally in favour of rog's suggestions, and I'm still a
bit confused at admin-secret slipping in in a couple of places. Maybe I just
misunderstood it?

https://codereview.appspot.com/12232043/diff/1/environs/azure/config.go
File environs/azure/config.go (right):

https://codereview.appspot.com/12232043/diff/1/environs/azure/config.go#newco...
environs/azure/config.go:155: m := make(map[string]interface{})
Would you run this by someone working on azure please? I'm not competent to
decide whether this is sane.

https://codereview.appspot.com/12232043/diff/1/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/12232043/diff/1/environs/config.go#newcode121
environs/config.go:121: err = cfg.MergeLocalState()
On 2013/08/02 10:51:07, rog wrote:
> I don't think it's right that ReadEnvironsBytes, which looks as if it parses a
> only a slice of bytes, goes off to read extra stuff from the user's home
> directory.
> 
> However, after discussion with William, I think we can leave it as is for the
> time being, but in the future we can move towards a model where we just open a
> single environment config, and we can then remove the entire Environs type
which
> is likely to be problematic in the future where we may be storing environment
> configs in the cloud.

I think this is the right place, but it's badly named. Environment loading in
this context always involves reading a bunch of random stuff from disk *anyway*
-- it's just that this change makes the existing crackfulness a bit more
immediately apparent. So, yeah, leave it as is and bear the concerns in mind as
it evolves.

https://codereview.appspot.com/12232043/diff/1/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/12232043/diff/1/environs/config/config.go#newc...
environs/config/config.go:452: func (cfg *Config) MergeLocalState() error {
I kinda feel this is the wrong way round for the long term, because one day the
local state file will be the only necessary source of truth for the environment,
but this is fine for now.

https://codereview.appspot.com/12232043/diff/1/environs/config/config.go#newc...
environs/config/config.go:461: cfg.m[k] = v
Hold on. I'm not sure about this.

(1) isn't there another map inside config.Config, into which some of the fields
should go? I understand that doesn't apply to admin-secret, though, so if it's
coming in a followup don't worry.

(2) I'm sad that configs are now mutable, and I would much prefer it if we could
use .Apply() to generate and return a fresh one. Are there unexpected
complexities there?

https://codereview.appspot.com/12232043/diff/1/environs/config/localstate.go
File environs/config/localstate.go (right):

https://codereview.appspot.com/12232043/diff/1/environs/config/localstate.go#...
environs/config/localstate.go:16: type LocalState struct {
On 2013/08/02 10:51:07, rog wrote:
> I'd be tempted to do simply:
> 
>     type LocalState map[string]interface{}
> 
> Unlike Config, there's nothing interesting
> to hide here.
> 
> Alternatively, we could just lose the LocalState
> type entirely and just use map[string]interface{}
> throughout. That would probably be my preference
> actually.

`type LocalState map[string]interface{}` is the sweet spot for me, I think.

https://codereview.appspot.com/12232043/diff/1/environs/config/localstate.go#...
environs/config/localstate.go:79: // If the local state file has not yet been
created, a nil instance is returned.
On 2013/08/02 10:51:07, rog wrote:
> 
> I think if the local state file hasn't been created, we
> should return (nil, not found error).
> 
> Returning (nil, nil) is asking for trouble IMO.
+1

https://codereview.appspot.com/12232043/diff/1/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/12232043/diff/1/environs/dummy/environs.go#new...
environs/dummy/environs.go:391: func (environProvider) LocalAttrs(cfg
*config.Config) map[string]interface{} {
I think this will evolve (lots of things will have this exact chunk of
functionality, with occasional type-specific additions) but fine for now again.

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

https://codereview.appspot.com/12232043/diff/1/environs/ec2/config.go#newcode45
environs/ec2/config.go:45: c.attrs["control-bucket"] = b
On 2013/08/02 10:51:07, rog wrote:
> 
> I think this side-effect from calling the controlBucket
> method is a little surprising. In particular it might
> come to bite us when we're trying to connect to an existing
> environment that doesn't happen to have a control bucket
> set or any local state - a random control bucket will be
> created which corresponds to no existing environment.
> 
> I'd prefer to have an initConfig method (or something similar)
> that's called explicitly in Bootstrap that fills out any
> config fields that are necessary.

Firm +1 here if we can do it. Helps us work towards a point where we can draw a
hard line between fuzzy-weird-user-config and
parsed-validated-completed-useful-config.

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

https://codereview.appspot.com/12232043/diff/1/environs/ec2/config_test.go#ne...
environs/ec2/config_test.go:111: c.Assert(localAttrs, DeepEquals, expectedLocal)
Wait, I'm definitely confused here. Shouldn't this include admin-secret?

https://codereview.appspot.com/12232043/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/12232043/diff/1/environs/interface.go#newcode46
environs/interface.go:46: LocalAttrs(cfg *config.Config) map[string]interface{}
On 2013/08/02 10:51:07, rog wrote:
> 
> Why doesn't this return *config.LocalAttrs ?

To rhyme with SecretAttrs, I guess. LocalAttrs would be nicer though.

[wallyworld, 2013-08-05 10:58:26]

I've refactored the implementation. A new interface HasLocalConfig is used to
determine whether a provider knows about local config attributes. A new Config
constructor NewWithLocal is used so that the previous merge method can be
eliminated. It's all a bit cleaner, and allows admin-secret to be processed
easily in the next branch.

https://codereview.appspot.com/12232043/diff/1/doc/glossary.txt
File doc/glossary.txt (right):

https://codereview.appspot.com/12232043/diff/1/doc/glossary.txt#newcode118
doc/glossary.txt:118: "access-key" and "secret-key" setting, but these can be
omitted if the
On 2013/08/02 10:51:07, rog wrote:
> s/setting/settings/

Done.

https://codereview.appspot.com/12232043/diff/1/environs/azure/config.go
File environs/azure/config.go (right):

https://codereview.appspot.com/12232043/diff/1/environs/azure/config.go#newco...
environs/azure/config.go:155: m := make(map[string]interface{})
On 2013/08/02 11:19:19, fwereade wrote:
> Would you run this by someone working on azure please? I'm not competent to
> decide whether this is sane.

The method is defined on the EnvironProvider interface. It allows any environ
provider to specify config attributes which should be stored locally. It's valid
for there to be no such attributes, in which case it's business as usual and no
local state file is written.

https://codereview.appspot.com/12232043/diff/1/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/12232043/diff/1/environs/config/config.go#newc...
environs/config/config.go:452: func (cfg *Config) MergeLocalState() error {
On 2013/08/02 11:19:19, fwereade wrote:
> I kinda feel this is the wrong way round for the long term, because one day
the
> local state file will be the only necessary source of truth for the
environment,
> but this is fine for now.

This method is now refactored.

https://codereview.appspot.com/12232043/diff/1/environs/config/localstate.go
File environs/config/localstate.go (right):

https://codereview.appspot.com/12232043/diff/1/environs/config/localstate.go#...
environs/config/localstate.go:16: type LocalState struct {
On 2013/08/02 11:19:19, fwereade wrote:
> On 2013/08/02 10:51:07, rog wrote:
> > I'd be tempted to do simply:
> > 
> >     type LocalState map[string]interface{}
> > 
> > Unlike Config, there's nothing interesting
> > to hide here.
> > 
> > Alternatively, we could just lose the LocalState
> > type entirely and just use map[string]interface{}
> > throughout. That would probably be my preference
> > actually.
> 
> `type LocalState map[string]interface{}` is the sweet spot for me, I think.

I do think a struct is better because I feel it breaks encapsulation otherwise,
but I used the type alias as suggested.

https://codereview.appspot.com/12232043/diff/1/environs/config/localstate.go#...
environs/config/localstate.go:65: logger.Warningf("permission of %q is %q",
stateDir, info.Mode().Perm())
On 2013/08/02 10:51:07, rog wrote:
> 
> I think this is overkill and just os.MkdirAll(stateDir, 0700) would do.
> if someone wants to manually chmod +w ~/.juju/.environments, they're free to
do
> so and i don't think a warning will help - they're unlikely to do it by
> accident.

This code was lifted from the code used to create .juju
I guess we can be a little more forgiving for stuff created inside .juju

https://codereview.appspot.com/12232043/diff/1/environs/config/localstate.go#...
environs/config/localstate.go:71: if err := os.Chmod(stateFilepath, 0600); err
!= nil {
On 2013/08/02 10:51:07, rog wrote:
> 
> 
> Likewise - it's quite possible that someone might *want* their environ files
to
> accessible to other users or groups on the same machine.

Done.

https://codereview.appspot.com/12232043/diff/1/environs/config/localstate.go#...
environs/config/localstate.go:79: // If the local state file has not yet been
created, a nil instance is returned.
On 2013/08/02 10:51:07, rog wrote:
> 
> I think if the local state file hasn't been created, we
> should return (nil, not found error).
> 
> Returning (nil, nil) is asking for trouble IMO.

Done.

https://codereview.appspot.com/12232043/diff/1/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/12232043/diff/1/environs/dummy/environs.go#new...
environs/dummy/environs.go:391: func (environProvider) LocalAttrs(cfg
*config.Config) map[string]interface{} {
On 2013/08/02 11:19:19, fwereade wrote:
> I think this will evolve (lots of things will have this exact chunk of
> functionality, with occasional type-specific additions) but fine for now
again.

This is now refactored. The admin-secrets bit is in a shared method.

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

https://codereview.appspot.com/12232043/diff/1/environs/ec2/config.go#newcode45
environs/ec2/config.go:45: c.attrs["control-bucket"] = b
On 2013/08/02 11:19:19, fwereade wrote:
> On 2013/08/02 10:51:07, rog wrote:
> > 
> > I think this side-effect from calling the controlBucket
> > method is a little surprising. In particular it might
> > come to bite us when we're trying to connect to an existing
> > environment that doesn't happen to have a control bucket
> > set or any local state - a random control bucket will be
> > created which corresponds to no existing environment.
> > 
> > I'd prefer to have an initConfig method (or something similar)
> > that's called explicitly in Bootstrap that fills out any
> > config fields that are necessary.
> 
> Firm +1 here if we can do it. Helps us work towards a point where we can draw
a
> hard line between fuzzy-weird-user-config and
> parsed-validated-completed-useful-config.

Done.

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

https://codereview.appspot.com/12232043/diff/1/environs/ec2/config_test.go#ne...
environs/ec2/config_test.go:111: c.Assert(localAttrs, DeepEquals, expectedLocal)
On 2013/08/02 11:19:19, fwereade wrote:
> Wait, I'm definitely confused here. Shouldn't this include admin-secret?

admin-secret is not extracted yet. only control-bucket

https://codereview.appspot.com/12232043/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/12232043/diff/1/environs/interface.go#newcode46
environs/interface.go:46: LocalAttrs(cfg *config.Config) map[string]interface{}
On 2013/08/02 11:19:19, fwereade wrote:
> On 2013/08/02 10:51:07, rog wrote:
> > 
> > Why doesn't this return *config.LocalAttrs ?
> 
> To rhyme with SecretAttrs, I guess. LocalAttrs would be nicer though.

Yes, I was being consistent. This is refactored a bit now anyway

https://codereview.appspot.com/12232043/diff/1/environs/openstack/config.go
File environs/openstack/config.go (right):

https://codereview.appspot.com/12232043/diff/1/environs/openstack/config.go#n...
environs/openstack/config.go:90: // control-bucket is not required in
environment.yaml anymore but we
On 2013/08/02 10:51:07, rog wrote:
> 
> Same concerns as for the ec2 equivalent.

Done.

[wallyworld, 2013-08-05 11:00:07]

Please take a look.

[rog, 2013-08-06 13:55:38]

Thanks for re-working this.
It's still not quite right though, in my view.

Summarising the comments below, I suggest the following:

1) add a LocalAttrs method to environs.Environ.

2) change ReadEnvironsBytes to call config.ReadLocalConfig explicitly, merge
with the attributes in environments.yaml, and call config.New with those.

3) change the providers so that they don't have control-bucket as a required
configuration attribute.

4) change the various provider Bootstrap methods to choose a name for the
control bucket if not set, and set this in the environ's configuration.

5) change the various provider environs to implement LocalAttrs.

Of these, 3) is going to be the most awkward, because current providers can
check for all the required configuration settings at Open time, so all entry
points will need changing to check that the configuration has any extra settings
that might be provided by Bootstrap.

Oh darn, I've just realised that it's not viable because when we're
bootstrapping we need to access the control bucket *before* calling Bootstrap.

I'll think this over for a bit more.

https://codereview.appspot.com/12232043/diff/15001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/12232043/diff/15001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:111: // Write out local state, if any, and if not already
written.
I don't know if writing "if not already written" is the right
thing to do here. Isn't it possible that a provider may
want to write out attributes specific to a particular
instantiated environment that might not be the
same the next time we bootstrap that environment?

I'm wondering what the down side is of just writing
the local state on bootstrap, regardless of whether
it already exists.

What do you think?

https://codereview.appspot.com/12232043/diff/15001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:113: if _, err =
config.ReadLocalConfig(environ.Config().Name()); err == nil {
s/=/:=/

https://codereview.appspot.com/12232043/diff/15001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:119: err =
config.WriteLocalConfig(environ.Config().Name(), ls)
s/=/:=/

https://codereview.appspot.com/12232043/diff/15001/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/12232043/diff/15001/cmd/juju/bootstrap_test.go...
cmd/juju/bootstrap_test.go:157: c.Check(map[string]interface{}(*lc),
gc.DeepEquals, map[string]interface{}{"local-attr": "dummy-local-attr"})
There's no test that the configuration is not overwritten if it already exists.
I'm not convinced of the merits of that behaviour, but if it's there, we should
test for it.

https://codereview.appspot.com/12232043/diff/15001/doc/glossary.txt
File doc/glossary.txt (right):

https://codereview.appspot.com/12232043/diff/15001/doc/glossary.txt#newcode118
doc/glossary.txt:118: "access-key" and "secret-key" settings, but these can be
omitted if the
Thanks for remembering to update this!

https://codereview.appspot.com/12232043/diff/15001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/12232043/diff/15001/environs/config/config.go#...
environs/config/config.go:26: type HasLocalConfig interface {
Given the name of the methods that this contains,
"HasLocalAttrs" would seem to be a better
fit. Alternatively, you could rename the methods,
replacing "Attrs" with "Config".

This type needs a doc comment. I also think that it
should live in environs/interface.go, along
with all the other interfaces that environ providers need
to implement (when someone talks about implementing a new provider,
it's very nice to be able to point to that file and say
"implement this").

See my comment in NewWithLocal.

https://codereview.appspot.com/12232043/diff/15001/environs/config/config.go#...
environs/config/config.go:30: InitLocalAttrs(cfgAttrs map[string]interface{})
LocalConfig
I don't see the need for this method.

Can't we initialise unspecified local attributes
when we call Environ.Bootstrap?

https://codereview.appspot.com/12232043/diff/15001/environs/config/config.go#...
environs/config/config.go:149: func NewWithLocal(provider interface{}, attrs
map[string]interface{}) (*Config, error) {
This doesn't seem right to me. This package should not be calling
provider-specific methods, and I don't think it needs to.

All this logic could live happily in the environs package,
and we'd only need the original New function here.

Specifically, the environs package could call ReadLocalConfig,
merge the attributes from that into the attributes from
environments.yaml, and call config.New with the result.

https://codereview.appspot.com/12232043/diff/15001/environs/config/config.go#...
environs/config/config.go:157: localCfg = initLocal(attrs, lc)
The way things are implemented now, this will overwrite
a control bucket setting even if one is explicitly set
in environments.yaml.

Is that really what we want?

This also goes against my suggestion that we fill out local attributes on
Bootstrap - this function is called whenever we open an environment,
which is not what we want.

https://codereview.appspot.com/12232043/diff/15001/environs/config/config.go#...
environs/config/config.go:181: func RandomKey() string {
We've already got utils.RandomBytes - that would be a better
place for this. Alternatively (and nicer, in my view) would
be to just use string(utils.RandomBytes(16)) directly
instead of referring to config.RandomKey.

https://codereview.appspot.com/12232043/diff/15001/environs/config/localconfi...
File environs/config/localconfig.go (right):

https://codereview.appspot.com/12232043/diff/15001/environs/config/localconfi...
environs/config/localconfig.go:22: func NewLocalConfig(attrs
map[string]interface{}) *LocalConfig {
Given that we're defining LocalConfig as a map, we should
pass it around by value, not by pointer.

See http://golang.org/pkg/net/url/#Values for an example.

That said, I don't really see why we need a new type
at all. It's just a bunch of key-value pairs
with no associated behaviour. A map[string]interface{}
seems like a reasonable way to represent that.

https://codereview.appspot.com/12232043/diff/15001/environs/config/localconfi...
environs/config/localconfig.go:47: if _, err = os.Lstat(stateDir);
os.IsNotExist(err) {
if err := os.MkdirAll(stateDir, 0700); err != nil {
     return err
}

(MkdirAll already checks whether the directory exists)

https://codereview.appspot.com/12232043/diff/15001/environs/config/localconfi...
environs/config/localconfig.go:64: return nil, errors.NotFoundf("%s does not
exist.", stateFilepath)
return nil, errors.NotFoundf("file %q", stateFilePath)

(NotFoundf itself appends "not found", and we don't generally have full stops at
the end of errors)

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

https://codereview.appspot.com/12232043/diff/15001/environs/ec2/config.go#new...
environs/ec2/config.go:97: if bucket, _ := attrs["control-bucket"].(string);
bucket != "" && ecfg.controlBucket() != bucket {
I don't understand this change.
The only time a configuration might have an unset control
bucket is before bootstrap, and in that case we should
not be calling Validate.

https://codereview.appspot.com/12232043/diff/15001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/12232043/diff/15001/environs/ec2/ec2.go#newcod...
environs/ec2/ec2.go:179: func (p environProvider) LocalAttrs(cfg *config.Config)
config.LocalConfig {
I think this method should be defined on the environ, not the environ provider.

I suggest, in the environs.Environ interface:

    // LocalAttrs returns any attributes defined by the
    // environment that should be stored independently
    // of the environments.yaml file.
    LocalAttrs() map[string]interface{}

I think that's all we need a provider to implement.

https://codereview.appspot.com/12232043/diff/15001/environs/ec2/ec2.go#newcod...
environs/ec2/ec2.go:180: m := make(map[string]interface{})
return map[string]interface{}{
    "control-bucket": cfg.UnknownAttrs()["control-bucket"],
}

https://codereview.appspot.com/12232043/diff/15001/environs/openstack/config.go
File environs/openstack/config.go (right):

https://codereview.appspot.com/12232043/diff/15001/environs/openstack/config....
environs/openstack/config.go:223: if controlBucket, _ :=
attrs["control-bucket"].(string); controlBucket != "" && ecfg.controlBucket() !=
controlBucket {
Same as in ec2.

https://codereview.appspot.com/12232043/diff/15001/environs/openstack/config_...
File environs/openstack/config_test.go (right):

https://codereview.appspot.com/12232043/diff/15001/environs/openstack/config_...
environs/openstack/config_test.go:133: expectedControlBucket :=
"juju-testenv-private-storage"
just inline the string?

[wallyworld, 2013-08-07 08:02:26]

Please take a look.

https://codereview.appspot.com/12232043/diff/15001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/12232043/diff/15001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:111: // Write out local state, if any, and if not already
written.
On 2013/08/06 13:55:39, rog wrote:
> I don't know if writing "if not already written" is the right
> thing to do here. Isn't it possible that a provider may
> want to write out attributes specific to a particular
> instantiated environment that might not be the
> same the next time we bootstrap that environment?
> 
> I'm wondering what the down side is of just writing
> the local state on bootstrap, regardless of whether
> it already exists.
> 
> What do you think?

I originally wrote it out all the time but then you said not to I think.
Once the local config is generated, it should not change the next time the env
is bootstrapped, because the control bucket etc is only generated once. There's
no harm in writing it out every time, so I'll do that.

https://codereview.appspot.com/12232043/diff/15001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:119: err =
config.WriteLocalConfig(environ.Config().Name(), ls)
On 2013/08/06 13:55:39, rog wrote:
> s/=/:=/

Done.

https://codereview.appspot.com/12232043/diff/15001/cmd/juju/bootstrap_test.go
File cmd/juju/bootstrap_test.go (right):

https://codereview.appspot.com/12232043/diff/15001/cmd/juju/bootstrap_test.go...
cmd/juju/bootstrap_test.go:157: c.Check(map[string]interface{}(*lc),
gc.DeepEquals, map[string]interface{}{"local-attr": "dummy-local-attr"})
On 2013/08/06 13:55:39, rog wrote:
> There's no test that the configuration is not overwritten if it already
exists.
> I'm not convinced of the merits of that behaviour, but if it's there, we
should
> test for it.

I can change the behaviour as per the comment in the bootstrap code.

https://codereview.appspot.com/12232043/diff/15001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/12232043/diff/15001/environs/config/config.go#...
environs/config/config.go:26: type HasLocalConfig interface {
On 2013/08/06 13:55:39, rog wrote:
> Given the name of the methods that this contains,
> "HasLocalAttrs" would seem to be a better
> fit. Alternatively, you could rename the methods,
> replacing "Attrs" with "Config".
> 

I like it how it is. Config is the generic terminology for what the interface
provides. The methods reflect the type of data.

> This type needs a doc comment. I also think that it
> should live in environs/interface.go, along
> with all the other interfaces that environ providers need
> to implement (when someone talks about implementing a new provider,
> it's very nice to be able to point to that file and say
> "implement this").
> 

Hmmm. Not sure I agree. This is an optional interface, that does not have to be
implemented at all if a particular provider does not require nor support local
config. To me it makes sense having it here since it deals with environ config.
I generally dislike shoving stuff into a big bit bucket, and prefer to declare
things in packages where it makes sense.

https://codereview.appspot.com/12232043/diff/15001/environs/config/config.go#...
environs/config/config.go:30: InitLocalAttrs(cfgAttrs map[string]interface{})
LocalConfig
On 2013/08/06 13:55:39, rog wrote:
> I don't see the need for this method.
> 
> Can't we initialise unspecified local attributes
> when we call Environ.Bootstrap?

Not that I can see. By the time environs.Bootstrap() is called, the environ has
been constructed, and the construction sets up the storage and other things
which require attrs from the local config. So the injection of the local config
needs to be done earlier. The place it is done in this branch makes sense to me
because it is happening as the yaml is loaded which is the basis of setting
things up. The yaml is loaded and instead of everything being in the yaml, it is
now augmented with local stuff.

https://codereview.appspot.com/12232043/diff/15001/environs/config/config.go#...
environs/config/config.go:149: func NewWithLocal(provider interface{}, attrs
map[string]interface{}) (*Config, error) {
On 2013/08/06 13:55:39, rog wrote:
> This doesn't seem right to me. This package should not be calling
> provider-specific methods, and I don't think it needs to.
> 
> All this logic could live happily in the environs package,
> and we'd only need the original New function here.
> 

> Specifically, the environs package could call ReadLocalConfig,
> merge the attributes from that into the attributes from
> environments.yaml, and call config.New with the result.

I moved the code as suggested.

https://codereview.appspot.com/12232043/diff/15001/environs/config/config.go#...
environs/config/config.go:157: localCfg = initLocal(attrs, lc)
On 2013/08/06 13:55:39, rog wrote:
> The way things are implemented now, this will overwrite
> a control bucket setting even if one is explicitly set
> in environments.yaml.
> 
> Is that really what we want?
> 

It doesn't do that. It on;y copies in local attrs if they are not there already.
And there are tests for that also.

> This also goes against my suggestion that we fill out local attributes on
> Bootstrap - this function is called whenever we open an environment,
> which is not what we want.

It's not called whenever we open an env - it's called prior to that when the env
yaml is parsed and the named envs are set up. ie when ReadEnvirons() is called.

https://codereview.appspot.com/12232043/diff/15001/environs/config/config.go#...
environs/config/config.go:181: func RandomKey() string {
On 2013/08/06 13:55:39, rog wrote:
> We've already got utils.RandomBytes - that would be a better
> place for this. Alternatively (and nicer, in my view) would
> be to just use string(utils.RandomBytes(16)) directly
> instead of referring to config.RandomKey.

Done.

https://codereview.appspot.com/12232043/diff/15001/environs/config/localconfi...
File environs/config/localconfig.go (right):

https://codereview.appspot.com/12232043/diff/15001/environs/config/localconfi...
environs/config/localconfig.go:22: func NewLocalConfig(attrs
map[string]interface{}) *LocalConfig {
On 2013/08/06 13:55:39, rog wrote:
> Given that we're defining LocalConfig as a map, we should
> pass it around by value, not by pointer.
> 
> See http://golang.org/pkg/net/url/#Values for an example.
> 
> That said, I don't really see why we need a new type
> at all. It's just a bunch of key-value pairs
> with no associated behaviour. A map[string]interface{}
> seems like a reasonable way to represent that.

I like encapsulating it as an opaque type for all the usual reasons. I really
don't want to expose the internals. Plus I have a feeling we may need to add
methods to it as well. Since it is a type, isn't returning a pointer the right
thing to do here. What if we make it a struct? We can change the implementation
and the calling code stays the same. So to me a constructor that returns a type
should always use a pointer so that the programming model is consistent.

https://codereview.appspot.com/12232043/diff/15001/environs/config/localconfi...
environs/config/localconfig.go:47: if _, err = os.Lstat(stateDir);
os.IsNotExist(err) {
On 2013/08/06 13:55:39, rog wrote:
> if err := os.MkdirAll(stateDir, 0700); err != nil {
>      return err
> }
> 
> (MkdirAll already checks whether the directory exists)

Done.

https://codereview.appspot.com/12232043/diff/15001/environs/config/localconfi...
environs/config/localconfig.go:64: return nil, errors.NotFoundf("%s does not
exist.", stateFilepath)
On 2013/08/06 13:55:39, rog wrote:
> return nil, errors.NotFoundf("file %q", stateFilePath)
> 
> (NotFoundf itself appends "not found", and we don't generally have full stops
at
> the end of errors)

Done.

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

https://codereview.appspot.com/12232043/diff/15001/environs/ec2/config.go#new...
environs/ec2/config.go:97: if bucket, _ := attrs["control-bucket"].(string);
bucket != "" && ecfg.controlBucket() != bucket {
On 2013/08/06 13:55:39, rog wrote:
> I don't understand this change.
> The only time a configuration might have an unset control
> bucket is before bootstrap, and in that case we should
> not be calling Validate.

I think this was an artifact of an older version. I reverted it

https://codereview.appspot.com/12232043/diff/15001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/12232043/diff/15001/environs/ec2/ec2.go#newcod...
environs/ec2/ec2.go:179: func (p environProvider) LocalAttrs(cfg *config.Config)
config.LocalConfig {
On 2013/08/06 13:55:39, rog wrote:
> I think this method should be defined on the environ, not the environ
provider.
> 
> I suggest, in the environs.Environ interface:
> 
>     // LocalAttrs returns any attributes defined by the
>     // environment that should be stored independently
>     // of the environments.yaml file.
>     LocalAttrs() map[string]interface{}
> 
> I think that's all we need a provider to implement.

It's on provider because that's what we have when ReadEnvironsBytes is called.
We are constructing environs at that stage, using the provider. I think it makes
sense anyway.

https://codereview.appspot.com/12232043/diff/15001/environs/ec2/ec2.go#newcod...
environs/ec2/ec2.go:180: m := make(map[string]interface{})
On 2013/08/06 13:55:39, rog wrote:
> return map[string]interface{}{
>     "control-bucket": cfg.UnknownAttrs()["control-bucket"],
> }

Done.

https://codereview.appspot.com/12232043/diff/15001/environs/openstack/config.go
File environs/openstack/config.go (right):

https://codereview.appspot.com/12232043/diff/15001/environs/openstack/config....
environs/openstack/config.go:223: if controlBucket, _ :=
attrs["control-bucket"].(string); controlBucket != "" && ecfg.controlBucket() !=
controlBucket {
On 2013/08/06 13:55:39, rog wrote:
> Same as in ec2.

Done.

[jameinel, 2013-08-08 18:14:04]

It is getting a bit late for me to claim I understand everything that is going
on here, though I like the path this is going.

I did want to put up some comments that I had while reading the code.

https://codereview.appspot.com/12232043/diff/25001/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/12232043/diff/25001/environs/config.go#newcode171
environs/config.go:171: //	attrs["admin-secret"] = adminSecret
I think we actually want admin-secret to be .juju/environments/*.yaml config and
not have it in environments.yaml. (And created as random data at bootstrap
time.)

Is there a reason you want users to have to think about the admin secret?

https://codereview.appspot.com/12232043/diff/25001/environs/localconfig.go
File environs/localconfig.go (right):

https://codereview.appspot.com/12232043/diff/25001/environs/localconfig.go#ne...
environs/localconfig.go:35: return config.JujuHomePath(".environments", envName)
Because the actual environment files are the point that needs to be shared
between people, I would probably recommend not making it private.

I realize it isn't something people should be editing, but we are already in
~/.juju so double-private seems a bit too much IMO.

[wallyworld, 2013-08-09 03:38:50]

https://codereview.appspot.com/12232043/diff/25001/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/12232043/diff/25001/environs/config.go#newcode171
environs/config.go:171: //	attrs["admin-secret"] = adminSecret
On 2013/08/08 18:14:04, jameinel wrote:
> I think we actually want admin-secret to be .juju/environments/*.yaml config
and
> not have it in environments.yaml. (And created as random data at bootstrap
> time.)
> 
> Is there a reason you want users to have to think about the admin secret?

The end goal is *not* to have admin-secret in the user yaml. It is only
commented out here because I am doing it in the very next branch but wanted to
illustrate where the code would live to show that attrs common to all providers
would be handled in once place and each provider would also do it's own as well.

https://codereview.appspot.com/12232043/diff/25001/environs/localconfig.go
File environs/localconfig.go (right):

https://codereview.appspot.com/12232043/diff/25001/environs/localconfig.go#ne...
environs/localconfig.go:35: return config.JujuHomePath(".environments", envName)
On 2013/08/08 18:14:04, jameinel wrote:
> Because the actual environment files are the point that needs to be shared
> between people, I would probably recommend not making it private.
> 
> I realize it isn't something people should be editing, but we are already in
> ~/.juju so double-private seems a bit too much IMO.

I am following the design doc by William, but there has been lively discussion
on this :-)