Boilerplate environments.yaml

Boilerplate environments.yaml

A new command "juju generate-config" is introduced to generate sensible config
for each registered provider. The Environs interface has a new method "BoilerPlateConfig()"
which is used to gather the config for each provider.

If a user has an environments.yaml already, it is not overwritten. Otherwise the boilerplate
has sensible defaults which just need a little tweaking to get a working system.

As new providers are rehistered (eg local, mass), the boilerplate header will need to be tweaked
to refer to the new providers.

As a driveby, since config is being generated, I've renamed auth-method to auth-mode. 
Python juju uses auth-mode and Go juju should be compatible.

https://code.launchpad.net/~wallyworld/juju-core/boilerplate-environment-setup/+merge/145769

(do not edit description out of merge proposal)

[wallyworld, 2013-01-31 04:30:00]

Please take a look.

[dimitern, 2013-01-31 09:26:06]

LGTM, nice!

[jameinel, 2013-01-31 09:45:02]

overall LGTM, a couple small points most of which can be treated as follow up
ideas.

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go
File environs/boilerplate_config.go (right):

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go#...
environs/boilerplate_config.go:23: admin-secret: <some made up number>
For these, I think it would be nice if the code that writes the boilerplate
would actually put data in here. So that you would get reasonably valid
defaults, and could just uncomment them and have good expectations of it
working.

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go#...
environs/boilerplate_config.go:33: admin-secret: <some made up number>
For secrets like this, I would like to see the code that generates the config
actually write random strings here.
That way you don't have to shell out to yet-another-program just to get a random
default.
We can put a comment next to it that users can feel free to change the value the
first time.

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go#...
environs/boilerplate_config.go:41: # Not required if env variable
AWS_ACCESS_KEY_ID is set
I would actually probably default these to commented out, and maybe change the
line to:

# The value usually comes from the environment variable AWS_ACCESS_KEY_ID
# It can be set here, though the env variable will override it
or
# It can be set here, to override the env variable

(Depending on what actually overrides what).

Or just

# Usually set via the env variable XXXX, but can be specified here

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

https://codereview.appspot.com/7223063/diff/1/environs/config.go#newcode153
environs/config.go:153: environsFile, err := environsPath(environsFile)
you might want to clarify that this is the filename vs being the actual file
object. I suppose the other code was already using 'environsFile', though.

I also feel like we might want a test suite setup hook that ensures we don't
actually write to the user's real environments.yaml. Any thoughts on that? I
think some tests set $HOME, but I don't think it is universally set/cleared.

[fwereade, 2013-01-31 14:02:23]

Looks good; some comments follow.

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

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode37
cmd/juju/bootstrap.go:37: if _, ok := err.(*os.PathError); ok {
I'd prefer to have a specific ErrNoConfig coming out of environs, rather than
assuming that any path error must indicate a missing config.

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode38
cmd/juju/bootstrap.go:38: fmt.Println("No juju enviroment configuration file
exists.")
Please use the streams provided in the cmd.Context -- that way you can test what
it actually wrote.

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode45
cmd/juju/bootstrap.go:45: return fmt.Errorf("A boilerplate environment
configurtion file could not be created: %s", err.Error())
s/figurt/figurat/

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go
File environs/boilerplate_config.go (right):

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go#...
environs/boilerplate_config.go:23: admin-secret: <some made up number>
On 2013/01/31 09:45:02, jameinel wrote:
> For these, I think it would be nice if the code that writes the boilerplate
> would actually put data in here. So that you would get reasonably valid
> defaults, and could just uncomment them and have good expectations of it
> working.

+1

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go#...
environs/boilerplate_config.go:33: admin-secret: <some made up number>
On 2013/01/31 09:45:02, jameinel wrote:
> For secrets like this, I would like to see the code that generates the config
> actually write random strings here.
> That way you don't have to shell out to yet-another-program just to get a
random
> default.
> We can put a comment next to it that users can feel free to change the value
the
> first time.

+1

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go#...
environs/boilerplate_config.go:41: # Not required if env variable
AWS_ACCESS_KEY_ID is set
On 2013/01/31 09:45:02, jameinel wrote:
> Or just
> 
> # Usually set via the env variable XXXX, but can be specified here

+1 to this in particular.

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go#...
environs/boilerplate_config.go:95: admin-secret: nothing
I'm pretty sure admin-secret is always required.

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

https://codereview.appspot.com/7223063/diff/1/environs/config.go#newcode153
environs/config.go:153: environsFile, err := environsPath(environsFile)
On 2013/01/31 09:45:02, jameinel wrote:
> you might want to clarify that this is the filename vs being the actual file
> object. I suppose the other code was already using 'environsFile', though.
> 
> I also feel like we might want a test suite setup hook that ensures we don't
> actually write to the user's real environments.yaml. Any thoughts on that? I
> think some tests set $HOME, but I don't think it is universally set/cleared.

Yeah, I'd prefer "path" or "name" for things that aren't actual Files.

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

https://codereview.appspot.com/7223063/diff/1/environs/config_test.go#newcode155
environs/config_test.go:155: outfile, err := environs.WriteEnvirons(path, env)
There should be a test for an empty path, and for what happens in that case when
$HOME is not set.

[rog, 2013-01-31 14:45:21]

thanks a lot for working on this.
i have some trivial comments, but a couple of substantive remarks - i'd be
interested in hearing what people think.

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

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode37
cmd/juju/bootstrap.go:37: if _, ok := err.(*os.PathError); ok {
On 2013/01/31 14:02:23, fwereade wrote:
> I'd prefer to have a specific ErrNoConfig coming out of environs, rather than
> assuming that any path error must indicate a missing config.

agreed. or at least it could os.IsNotExist.

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode39
cmd/juju/bootstrap.go:39: filename, err := environs.WriteEnvirons("",
environs.BoilerPlateConfig())
i know that this is legacy behaviour, but i'd much prefer to see a separate
explicit command that's responsible for generating a default environment, say
juju generate-environment.

rather than actually generating the environment when it's not found, we could
print:

    No juju environment configuration exists.
    Please create a configuration by running
       juju generate-environment
    then edit the file to configure your juju environment.

or something like that.

The separate command is actually independently useful because we'll want to
generate new environment settings when a new providers is added - even if we
already have an existing environments.yaml. Having a separate command makes it
easy to have that functionality without needing to remove environments.yaml just
to trigger the "not found" functionality of bootstrap.

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode41
cmd/juju/bootstrap.go:41: fmt.Printf("A boilerplate environment configuration
file has been written to %s.\n", filename)
if we're using Println below, we shouldn't use Printf here. or vice versa.

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode43
cmd/juju/bootstrap.go:43: return nil
i think this should return an error - the environment has not successfully
bootstrapped.

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go
File environs/boilerplate_config.go (right):

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go#...
environs/boilerplate_config.go:31: amazon:
rather than have an entry for each provider in this file, i'd prefer to have
environs iterate through the registered environment types and have each one
generate its own default configuration.

the easiest would be for it to return it as a map[string]interface{},
then the environs code could integrate them all into one file, but in fact it
would probably have to return it as text because we want inline comments. if we
decided that a single (multiline) comment per provider would be sufficient, the
first approach might work ok - we'd have a provider return default settings and
some comment text.

one particular reason for this is that it does not make sense, IMHO, to generate
an environments.yaml file that references environments not available with the
juju tool that's being used.

it also means that people can register custom environment provider types and
have the juju tool just work, generating a default environments.yaml that
contains configuration for the custom environment too.

thoughts?

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

https://codereview.appspot.com/7223063/diff/1/environs/config.go#newcode157
environs/config.go:157: err = ioutil.WriteFile(environsFile,
[]byte(fileContents), 0666)
generally, if err is non-nil, we make sure that the other value is zero, so
if err := ...; err != nil {
    return "", nil
}
return environsFile, nil

[fwereade, 2013-01-31 15:02:36]

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

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode39
cmd/juju/bootstrap.go:39: filename, err := environs.WriteEnvirons("",
environs.BoilerPlateConfig())
On 2013/01/31 14:45:21, rog wrote:
> i know that this is legacy behaviour, but i'd much prefer to see a separate
> explicit command that's responsible for generating a default environment, say
> juju generate-environment.
> 
> rather than actually generating the environment when it's not found, we could
> print:
> 
>     No juju environment configuration exists.
>     Please create a configuration by running
>        juju generate-environment
>     then edit the file to configure your juju environment.
> 
> or something like that.
> 
> The separate command is actually independently useful because we'll want to
> generate new environment settings when a new providers is added - even if we
> already have an existing environments.yaml. Having a separate command makes it
> easy to have that functionality without needing to remove environments.yaml
just
> to trigger the "not found" functionality of bootstrap.
> 

+1

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode43
cmd/juju/bootstrap.go:43: return nil
On 2013/01/31 14:45:21, rog wrote:
> i think this should return an error - the environment has not successfully
> bootstrapped.
+1

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go
File environs/boilerplate_config.go (right):

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go#...
environs/boilerplate_config.go:31: amazon:
On 2013/01/31 14:45:21, rog wrote:
> rather than have an entry for each provider in this file, i'd prefer to have
> environs iterate through the registered environment types and have each one
> generate its own default configuration.
> 
> the easiest would be for it to return it as a map[string]interface{},
> then the environs code could integrate them all into one file, but in fact it
> would probably have to return it as text because we want inline comments. if
we
> decided that a single (multiline) comment per provider would be sufficient,
the
> first approach might work ok - we'd have a provider return default settings
and
> some comment text.

A short comment explaining what *must* be filled in, plus a URL for more
information, might be about right. Ofc that can all be put into a single
"comment" string. 

> 
> one particular reason for this is that it does not make sense, IMHO, to
generate
> an environments.yaml file that references environments not available with the
> juju tool that's being used.
> 
> it also means that people can register custom environment provider types and
> have the juju tool just work, generating a default environments.yaml that
> contains configuration for the custom environment too.
> 
> thoughts?

Overall +1.

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

https://codereview.appspot.com/7223063/diff/1/environs/config.go#newcode157
environs/config.go:157: err = ioutil.WriteFile(environsFile,
[]byte(fileContents), 0666)
On 2013/01/31 14:45:21, rog wrote:
> generally, if err is non-nil, we make sure that the other value is zero, so
> if err := ...; err != nil {
>     return "", nil
> }
> return environsFile, nil

+1

[wallyworld, 2013-02-01 11:52:56]

Please take a look.

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

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode37
cmd/juju/bootstrap.go:37: if _, ok := err.(*os.PathError); ok {
On 2013/01/31 14:45:21, rog wrote:
> On 2013/01/31 14:02:23, fwereade wrote:
> > I'd prefer to have a specific ErrNoConfig coming out of environs, rather
than
> > assuming that any path error must indicate a missing config.
> 
> agreed. or at least it could os.IsNotExist.

I used os.IsNotExist. I didn't know about this API so thanks for suggesting it.

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode38
cmd/juju/bootstrap.go:38: fmt.Println("No juju enviroment configuration file
exists.")
On 2013/01/31 14:02:23, fwereade wrote:
> Please use the streams provided in the cmd.Context -- that way you can test
what
> it actually wrote.

Thanks for the tip. I didn't know they existed.

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode39
cmd/juju/bootstrap.go:39: filename, err := environs.WriteEnvirons("",
environs.BoilerPlateConfig())
On 2013/01/31 14:45:21, rog wrote:
> i know that this is legacy behaviour, but i'd much prefer to see a separate
> explicit command that's responsible for generating a default environment, say
> juju generate-environment.
> 
> rather than actually generating the environment when it's not found, we could
> print:
> 
>     No juju environment configuration exists.
>     Please create a configuration by running
>        juju generate-environment
>     then edit the file to configure your juju environment.
> 
> or something like that.
> 
> The separate command is actually independently useful because we'll want to
> generate new environment settings when a new providers is added - even if we
> already have an existing environments.yaml. Having a separate command makes it
> easy to have that functionality without needing to remove environments.yaml
just
> to trigger the "not found" functionality of bootstrap.
> 

Done.

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode39
cmd/juju/bootstrap.go:39: filename, err := environs.WriteEnvirons("",
environs.BoilerPlateConfig())
On 2013/01/31 14:45:21, rog wrote:
> i know that this is legacy behaviour, but i'd much prefer to see a separate
> explicit command that's responsible for generating a default environment, say
> juju generate-environment.
> 
> rather than actually generating the environment when it's not found, we could
> print:
> 
>     No juju environment configuration exists.
>     Please create a configuration by running
>        juju generate-environment
>     then edit the file to configure your juju environment.
> 
> or something like that.
> 
> The separate command is actually independently useful because we'll want to
> generate new environment settings when a new providers is added - even if we
> already have an existing environments.yaml. Having a separate command makes it
> easy to have that functionality without needing to remove environments.yaml
just
> to trigger the "not found" functionality of bootstrap.
> 

I thought about doing this and wasn't sure if I was allowed to change legacy
behaviour, so I'm really pleased you want to do it this way. I would suggest
that we make the command "generate-config" since we already have a
"destroy-environment" which actually deletes an instance and I think it's best
to use different terminology so the user doesn't get confused.

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode41
cmd/juju/bootstrap.go:41: fmt.Printf("A boilerplate environment configuration
file has been written to %s.\n", filename)
On 2013/01/31 14:45:21, rog wrote:
> if we're using Println below, we shouldn't use Printf here. or vice versa.

Why is that? Println doesn't offer the format capability but does do the \n
without having to insert it in the string. So it was convenient to use Println
where there was no value substitutions required. It it a juju project coding
preference or a Go idiom or...?

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode43
cmd/juju/bootstrap.go:43: return nil
On 2013/01/31 15:02:36, fwereade wrote:
> On 2013/01/31 14:45:21, rog wrote:
> > i think this should return an error - the environment has not successfully
> > bootstrapped.
> +1

I'll move this stuff to a separate commnd as suggested elsewhere.

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode45
cmd/juju/bootstrap.go:45: return fmt.Errorf("A boilerplate environment
configurtion file could not be created: %s", err.Error())
On 2013/01/31 14:02:23, fwereade wrote:
> s/figurt/figurat/

Done.

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go
File environs/boilerplate_config.go (right):

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go#...
environs/boilerplate_config.go:23: admin-secret: <some made up number>
On 2013/01/31 09:45:02, jameinel wrote:
> For these, I think it would be nice if the code that writes the boilerplate
> would actually put data in here. So that you would get reasonably valid
> defaults, and could just uncomment them and have good expectations of it
> working.

Done.

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go#...
environs/boilerplate_config.go:31: amazon:
On 2013/01/31 14:45:21, rog wrote:
> rather than have an entry for each provider in this file, i'd prefer to have
> environs iterate through the registered environment types and have each one
> generate its own default configuration.
> 
> the easiest would be for it to return it as a map[string]interface{},
> then the environs code could integrate them all into one file, but in fact it
> would probably have to return it as text because we want inline comments. if
we
> decided that a single (multiline) comment per provider would be sufficient,
the
> first approach might work ok - we'd have a provider return default settings
and
> some comment text.
> 
> one particular reason for this is that it does not make sense, IMHO, to
generate
> an environments.yaml file that references environments not available with the
> juju tool that's being used.
> 
> it also means that people can register custom environment provider types and
> have the juju tool just work, generating a default environments.yaml that
> contains configuration for the custom environment too.
> 
> thoughts?

This is a nice idea but there is a problem. There are variants of each
environment type with different suggested config settings. eg we currently have
generic openstack and HP openstack with differences in the port number for the
auth URL for example. So there's not a 1-1 between the provider types and the
entries in the boilerplate config file. 

I also prefer the comments in the boilerplate file next to the various config
entries rather than just a block comment at the top so would want the
map[string]string variant suggested. But is there anything wrong with the raw
string approach? I like it because it can be explicitly laid out visually to
make a nice example file.

I've made it so that each provider can produce a raw string representing it's
example boilerplate configuration. These are appended to a boilerplate config
header section which is then written out to environments.yaml.

What do you think?

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go#...
environs/boilerplate_config.go:33: admin-secret: <some made up number>
On 2013/01/31 09:45:02, jameinel wrote:
> For secrets like this, I would like to see the code that generates the config
> actually write random strings here.
> That way you don't have to shell out to yet-another-program just to get a
random
> default.
> We can put a comment next to it that users can feel free to change the value
the
> first time.

Done.

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go#...
environs/boilerplate_config.go:41: # Not required if env variable
AWS_ACCESS_KEY_ID is set
On 2013/01/31 09:45:02, jameinel wrote:
> I would actually probably default these to commented out, and maybe change the
> line to:
> 
> # The value usually comes from the environment variable AWS_ACCESS_KEY_ID
> # It can be set here, though the env variable will override it
> or
> # It can be set here, to override the env variable
> 
> (Depending on what actually overrides what).
> 
> Or just
> 
> # Usually set via the env variable XXXX, but can be specified here

Done.

https://codereview.appspot.com/7223063/diff/1/environs/boilerplate_config.go#...
environs/boilerplate_config.go:95: admin-secret: nothing
On 2013/01/31 14:02:23, fwereade wrote:
> I'm pretty sure admin-secret is always required.

I got the value 'nothing' from the mass juju quick start website. Perhaps maas
doesn't care what the value is, so they suggest 'nothing'? I'll change it to a
random string like for the others

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

https://codereview.appspot.com/7223063/diff/1/environs/config.go#newcode153
environs/config.go:153: environsFile, err := environsPath(environsFile)
On 2013/01/31 09:45:02, jameinel wrote:
> you might want to clarify that this is the filename vs being the actual file
> object. I suppose the other code was already using 'environsFile', though.
> 

I altered the new code and fixed the existing code.

> I also feel like we might want a test suite setup hook that ensures we don't
> actually write to the user's real environments.yaml. Any thoughts on that? I
> think some tests set $HOME, but I don't think it is universally set/cleared.

The tests I looked at all set up a fake home and manually used ioutil.WriteFile
to write out the test environs file. All that is being done here is the
ioutil.WriteFile call is being encapsulated inside a function which the tests
have been changed to use, and some duplicate code snippets removed. Every test
which writes to environments.yaml sets up a fake home first.

https://codereview.appspot.com/7223063/diff/1/environs/config.go#newcode157
environs/config.go:157: err = ioutil.WriteFile(environsFile,
[]byte(fileContents), 0666)
On 2013/01/31 14:45:21, rog wrote:
> generally, if err is non-nil, we make sure that the other value is zero, so
> if err := ...; err != nil {
>     return "", nil
> }
> return environsFile, nil

Done.

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

https://codereview.appspot.com/7223063/diff/1/environs/config_test.go#newcode155
environs/config_test.go:155: outfile, err := environs.WriteEnvirons(path, env)
On 2013/01/31 14:02:23, fwereade wrote:
> There should be a test for an empty path, and for what happens in that case
when
> $HOME is not set.

I added a test for no $HOME. I modified this test for the empty (default) path
case.

[rog, 2013-02-01 13:49:09]

looking good. mostly trivials here, with a couple of slightly more significant
queries.

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

https://codereview.appspot.com/7223063/diff/1/cmd/juju/bootstrap.go#newcode41
cmd/juju/bootstrap.go:41: fmt.Printf("A boilerplate environment configuration
file has been written to %s.\n", filename)
On 2013/02/01 11:52:56, wallyworld wrote:
> On 2013/01/31 14:45:21, rog wrote:
> > if we're using Println below, we shouldn't use Printf here. or vice versa.
> 
> Why is that? Println doesn't offer the format capability but does do the \n
> without having to insert it in the string. So it was convenient to use Println
> where there was no value substitutions required. It it a juju project coding
> preference or a Go idiom or...?

just for consistency. each was capable of doing the task of the other:

    fmt.Printf("foo: %s\n", x)

is exactly equivalent to:

    fmt.Println("foo:", x)

i think it's easier to see what's happening if all similar operations in a
sequence are as similar as possible. YMMV of course!

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

https://codereview.appspot.com/7223063/diff/9001/cmd/juju/bootstrap.go#newcode36
cmd/juju/bootstrap.go:36: if err != nil {
you could do this to lose a couple of indentation levels
from the printfs:

if err == nil {
   return environs.Bootstrap(environ, c.UploadTools, nil) 
}
if !os.IsNotExist(err) {
   return err
}
... println

https://codereview.appspot.com/7223063/diff/9001/cmd/juju/generateconfig.go
File cmd/juju/generateconfig.go (right):

https://codereview.appspot.com/7223063/diff/9001/cmd/juju/generateconfig.go#n...
cmd/juju/generateconfig.go:45: fmt.Fprintf(out, "It will not be overwritten.\n")
i'd like to see a way to see the generated environment even if i've already got
an environments.yaml file. one way would be to add a "-o" option.

e.g. juju generate-config -o -
to print to stdout.

another possibility is to make the command write
the environment config only when explicitly
requested.

e.g. juju generate-config -w

would give the current behaviour - if -w is not provided, it would print to
stdout like a nice well behaved unix command :-).

if we chose that path, the command printed by bootstrap would include the "-w"
flag, of course.

thoughts?

https://codereview.appspot.com/7223063/diff/9001/environs/boilerplate_config.go
File environs/boilerplate_config.go (right):

https://codereview.appspot.com/7223063/diff/9001/environs/boilerplate_config....
environs/boilerplate_config.go:21: controlBucketKey = "${control-bucket}"
i'm not sure it's appropriate to have these quite environment-specific things in
this general package.

i wonder if we might provide a single primitive, "rand", and use the template
package to generate the final boilerplate, rather than ad hoc use of
strings.Replace.

i don't think we mind particularly whether control bucket names and admin
secrets are the same across all environments - in fact it's perhaps better if
they're not.

so BoilerplateConfig, below, would become something like:

var config bytes.Buffer
config.WriteString(configHeader)
for _, p := range providers {
    t, err := parseTemplate(p.BoilerplateConfig())
    if err != nil {
        panic(fmt.Errorf("cannot parse boilerplate from %s: %v", p.Name(), err))
    }
    var ecfg bytes.Buffer
    if err := t.Execute(&ecfg, nil); err != nil {
       panic(...)
    }
    indent(&config, ecfg.Bytes(), "  ")
}
return config.String()

and the two supporting functions could be (adjust to taste):

func parseTemplate(s string) (*template.Template, error) {
	t := template.New("").
	t.Funcs(template.FuncMap{"rand": randomKey})
	return t.Parse(s)
}

// indent appends the given text to the given buffer
// indented by the given ident string.
func indent(b *bytes.Buffer, s []byte, indent string) {
	for {
		if len(s) == 0 {
			return
		}
		b.WriteString(indent)
		i := bytes.IndexByte(s, '\n')
		if i == -1 {
			b.Write(s)
			b.WriteRune('\n')
			return
		}
		i++
		b.Write(s[0:i])
		s = s[i:]
	}
}

https://codereview.appspot.com/7223063/diff/9001/environs/boilerplate_config....
environs/boilerplate_config.go:38: // BoilerPlateConfig returns a sample juju
configuration which is written to a boilerplate environments.yaml file.
s/ which is written.*/./

(we can't say here if it's written or not)

https://codereview.appspot.com/7223063/diff/9001/environs/boilerplate_config....
environs/boilerplate_config.go:39: func BoilerPlateConfig() string {
s/BoilerPlate/Boilerplate/

https://codereview.appspot.com/7223063/diff/9001/environs/boilerplate_config....
environs/boilerplate_config.go:65: return config
is it feasible to check that the resulting configuration actually parses as
valid yaml?

https://codereview.appspot.com/7223063/diff/9001/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/7223063/diff/9001/environs/config.go#newcode119
environs/config.go:119: func environsPath(environsFilepath string) (string,
error) {
s/environsFilepath/path/ ?

https://codereview.appspot.com/7223063/diff/9001/environs/config.go#newcode133
environs/config.go:133: // If environsFile is empty,
$HOME/.juju/environments.yaml
environsFile isn't right any more.
how about simply "path" below too.

https://codereview.appspot.com/7223063/diff/9001/environs/config.go#newcode152
environs/config.go:152: func WriteEnvirons(environsFilepath string, fileContents
string) (string, error) {
"path" here too?

https://codereview.appspot.com/7223063/diff/9001/environs/config.go#newcode157
environs/config.go:157: if err = ioutil.WriteFile(environsFilepath,
[]byte(fileContents), 0666); err != nil {
erm, better to go with returning the error...
(i know my suggested program text was wrong, sorry about that!)

better to declare things as locally as possible too, i think (err :=)

https://codereview.appspot.com/7223063/diff/9001/environs/config.go#newcode160
environs/config.go:160: return environsFilepath, err
s/err/nil/

https://codereview.appspot.com/7223063/diff/9001/environs/config_test.go
File environs/config_test.go (right):

https://codereview.appspot.com/7223063/diff/9001/environs/config_test.go#newc...
environs/config_test.go:189: defer makeMissingHome(c).restore()
or:

defer makeFakeHome(c).restore()
os.Setenv("HOME", "")

marginally less efficient, but less lines of code :-)

https://codereview.appspot.com/7223063/diff/9001/environs/config_test.go#newc...
environs/config_test.go:199: c.Assert(err, Not(IsNil))
better to use NotNil rather than Not(IsNil), but in this case, i think better
still to check the actual error text.

https://codereview.appspot.com/7223063/diff/9001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/7223063/diff/9001/environs/dummy/environs.go#n...
environs/dummy/environs.go:388: return ""
i think we should return a suitable boilerplate config here too, so that the
environs boilerplate tests are working with at least one configuration.

https://codereview.appspot.com/7223063/diff/9001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/7223063/diff/9001/environs/interface.go#newcode23
environs/interface.go:23: // BoilerPlateConfig returns a yaml format string
which contains boilerplate
i'd like to avoid repeating the word "boilerplate" in the description (also,
it's one word so the capital "p" isn't appropriate). how about:

// Boilerplate returns a default configuration
// for the environment in yaml format. The text
// should be have a key followed by some number
// of attributes:
//
//   `environName:
//      type: environTypeName
//      attr1: val1
//   `
//
// The text is used as a template (see the template package)
// with one extra template function available, rand,
// which expands to a random hexadecimal string
// when invoked.

(i think it's nicer to have the environs functions return the
text unindented - it means they don't need to know
the exact level of indentation of the environments
yaml, which really should be specific to the environs package.)

https://codereview.appspot.com/7223063/diff/9001/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/7223063/diff/9001/environs/openstack/provider....
environs/openstack/provider.go:89: # globally unique swift bucket name
s/globally/Globally/ ?

https://codereview.appspot.com/7223063/diff/9001/environs/openstack/provider....
environs/openstack/provider.go:102: `
s/`/`[1:]

(do we want to see an extra blank line at the start?)

[wallyworld, 2013-02-04 01:25:05]

Please take a look.

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

https://codereview.appspot.com/7223063/diff/9001/cmd/juju/bootstrap.go#newcode36
cmd/juju/bootstrap.go:36: if err != nil {
On 2013/02/01 13:49:10, rog wrote:
> you could do this to lose a couple of indentation levels
> from the printfs:
> 
> if err == nil {
>    return environs.Bootstrap(environ, c.UploadTools, nil) 
> }
> if !os.IsNotExist(err) {
>    return err
> }
> ... println
> 

Done.

https://codereview.appspot.com/7223063/diff/9001/cmd/juju/generateconfig.go
File cmd/juju/generateconfig.go (right):

https://codereview.appspot.com/7223063/diff/9001/cmd/juju/generateconfig.go#n...
cmd/juju/generateconfig.go:45: fmt.Fprintf(out, "It will not be overwritten.\n")
On 2013/02/01 13:49:10, rog wrote:
> i'd like to see a way to see the generated environment even if i've already
got
> an environments.yaml file. one way would be to add a "-o" option.
> 
> e.g. juju generate-config -o -
> to print to stdout.
> 
> another possibility is to make the command write
> the environment config only when explicitly
> requested.
> 
> e.g. juju generate-config -w
> 
> would give the current behaviour - if -w is not provided, it would print to
> stdout like a nice well behaved unix command :-).
> 
> if we chose that path, the command printed by bootstrap would include the "-w"
> flag, of course.
> 
> thoughts?

This is a good idea. I've implemented it using the "-w" suggestion.

https://codereview.appspot.com/7223063/diff/9001/environs/boilerplate_config.go
File environs/boilerplate_config.go (right):

https://codereview.appspot.com/7223063/diff/9001/environs/boilerplate_config....
environs/boilerplate_config.go:21: controlBucketKey = "${control-bucket}"
On 2013/02/01 13:49:10, rog wrote:

I've modified as suggested to use the template package. I didn't realise that
existed.

https://codereview.appspot.com/7223063/diff/9001/environs/boilerplate_config....
environs/boilerplate_config.go:38: // BoilerPlateConfig returns a sample juju
configuration which is written to a boilerplate environments.yaml file.
On 2013/02/01 13:49:10, rog wrote:
> s/ which is written.*/./
> 
> (we can't say here if it's written or not)

Done.

https://codereview.appspot.com/7223063/diff/9001/environs/boilerplate_config....
environs/boilerplate_config.go:39: func BoilerPlateConfig() string {
On 2013/02/01 13:49:10, rog wrote:
> s/BoilerPlate/Boilerplate/

Done.

https://codereview.appspot.com/7223063/diff/9001/environs/boilerplate_config....
environs/boilerplate_config.go:65: return config
On 2013/02/01 13:49:10, rog wrote:
> is it feasible to check that the resulting configuration actually parses as
> valid yaml?

Done.

https://codereview.appspot.com/7223063/diff/9001/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/7223063/diff/9001/environs/config.go#newcode119
environs/config.go:119: func environsPath(environsFilepath string) (string,
error) {
On 2013/02/01 13:49:10, rog wrote:
> s/environsFilepath/path/ ?

Done.

https://codereview.appspot.com/7223063/diff/9001/environs/config.go#newcode133
environs/config.go:133: // If environsFile is empty,
$HOME/.juju/environments.yaml
On 2013/02/01 13:49:10, rog wrote:
> environsFile isn't right any more.
> how about simply "path" below too.

Done.

https://codereview.appspot.com/7223063/diff/9001/environs/config.go#newcode152
environs/config.go:152: func WriteEnvirons(environsFilepath string, fileContents
string) (string, error) {
On 2013/02/01 13:49:10, rog wrote:
> "path" here too?

Done.

https://codereview.appspot.com/7223063/diff/9001/environs/config.go#newcode157
environs/config.go:157: if err = ioutil.WriteFile(environsFilepath,
[]byte(fileContents), 0666); err != nil {
On 2013/02/01 13:49:10, rog wrote:
> erm, better to go with returning the error...
> (i know my suggested program text was wrong, sorry about that!)
> 
> better to declare things as locally as possible too, i think (err :=)

Done.

https://codereview.appspot.com/7223063/diff/9001/environs/config.go#newcode160
environs/config.go:160: return environsFilepath, err
On 2013/02/01 13:49:10, rog wrote:
> s/err/nil/

Done.

https://codereview.appspot.com/7223063/diff/9001/environs/config_test.go
File environs/config_test.go (right):

https://codereview.appspot.com/7223063/diff/9001/environs/config_test.go#newc...
environs/config_test.go:189: defer makeMissingHome(c).restore()
On 2013/02/01 13:49:10, rog wrote:
> or:
> 
> defer makeFakeHome(c).restore()
> os.Setenv("HOME", "")
> 
> marginally less efficient, but less lines of code :-)

Done.

https://codereview.appspot.com/7223063/diff/9001/environs/config_test.go#newc...
environs/config_test.go:199: c.Assert(err, Not(IsNil))
On 2013/02/01 13:49:10, rog wrote:
> better to use NotNil rather than Not(IsNil), but in this case, i think better
> still to check the actual error text.

Done.

https://codereview.appspot.com/7223063/diff/9001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/7223063/diff/9001/environs/dummy/environs.go#n...
environs/dummy/environs.go:388: return ""
On 2013/02/01 13:49:10, rog wrote:
> i think we should return a suitable boilerplate config here too, so that the
> environs boilerplate tests are working with at least one configuration.

Done.

https://codereview.appspot.com/7223063/diff/9001/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/7223063/diff/9001/environs/interface.go#newcode23
environs/interface.go:23: // BoilerPlateConfig returns a yaml format string
which contains boilerplate
On 2013/02/01 13:49:10, rog wrote:
> i'd like to avoid repeating the word "boilerplate" in the description (also,
> it's one word so the capital "p" isn't appropriate). how about:
> 
> // Boilerplate returns a default configuration
> // for the environment in yaml format. The text
> // should be have a key followed by some number
> // of attributes:
> //
> //   `environName:
> //      type: environTypeName
> //      attr1: val1
> //   `
> //
> // The text is used as a template (see the template package)
> // with one extra template function available, rand,
> // which expands to a random hexadecimal string
> // when invoked.
> 
> (i think it's nicer to have the environs functions return the
> text unindented - it means they don't need to know
> the exact level of indentation of the environments
> yaml, which really should be specific to the environs package.)
> 

Done.

https://codereview.appspot.com/7223063/diff/9001/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/7223063/diff/9001/environs/openstack/provider....
environs/openstack/provider.go:89: # globally unique swift bucket name
On 2013/02/01 13:49:10, rog wrote:
> s/globally/Globally/ ?

Done.

https://codereview.appspot.com/7223063/diff/9001/environs/openstack/provider....
environs/openstack/provider.go:102: `
On 2013/02/01 13:49:10, rog wrote:
> s/`/`[1:]
> 
> (do we want to see an extra blank line at the start?)

Done.

[rog, 2013-02-04 09:39:25]

LGTM with the below minor issues addressed.

thanks a lot for bearing with me through this!

please wait for at least one other to review the code though - it's changed
quite a bit.

https://codereview.appspot.com/7223063/diff/7003/environs/boilerplate_config.go
File environs/boilerplate_config.go (right):

https://codereview.appspot.com/7223063/diff/7003/environs/boilerplate_config....
environs/boilerplate_config.go:32: panic(fmt.Sprintf("error from crypto rand:
%v", err))
s/Sprintf/Errorf/

just convention - not that it makes much difference.

https://codereview.appspot.com/7223063/diff/7003/environs/boilerplate_config....
environs/boilerplate_config.go:51: indent(&config, ecfg.Bytes(), " ")
i think we use two-space indent elsewhere, so probably good to use here too.

https://codereview.appspot.com/7223063/diff/7003/environs/boilerplate_config....
environs/boilerplate_config.go:68: // indent appends the given text to the given
buffer indented by the given ident string.
s/ident/indent/

https://codereview.appspot.com/7223063/diff/7003/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/7223063/diff/7003/environs/dummy/environs.go#n...
environs/dummy/environs.go:390: dummy:
we don't need to indent this here any more.

https://codereview.appspot.com/7223063/diff/7003/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/7223063/diff/7003/environs/ec2/ec2.go#newcode112
environs/ec2/ec2.go:112: amazon:
no indent required.

https://codereview.appspot.com/7223063/diff/7003/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/7223063/diff/7003/environs/interface.go#newcode24
environs/interface.go:24: // The text should be have a key followed by some
number of attributes:
s/be have/be/

oops

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

https://codereview.appspot.com/7223063/diff/7003/environs/openstack/config.go...
environs/openstack/config.go:17: "auth-mode":         schema.String(),
i don't think this change should be here. but it may well be a codereview
artifact - ignore me if this is a change that's already gone into trunk.

https://codereview.appspot.com/7223063/diff/7003/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/7223063/diff/7003/environs/openstack/provider....
environs/openstack/provider.go:64: openstack:
we can lose the indent here.

[fwereade, 2013-02-04 15:49:16]

LGTM; otherwise general +1 to rog's suggestions. Thanks for this.

https://codereview.appspot.com/7223063/diff/7003/cmd/juju/generateconfig.go
File cmd/juju/generateconfig.go (right):

https://codereview.appspot.com/7223063/diff/7003/cmd/juju/generateconfig.go#n...
cmd/juju/generateconfig.go:51: fmt.Fprint(out, "Edit the file to configure your
juju environment and re-run bootstrap.\n")
s/re-run/run/

[wallyworld, 2013-02-04 23:45:30]

https://codereview.appspot.com/7223063/diff/7003/cmd/juju/generateconfig.go
File cmd/juju/generateconfig.go (right):

https://codereview.appspot.com/7223063/diff/7003/cmd/juju/generateconfig.go#n...
cmd/juju/generateconfig.go:51: fmt.Fprint(out, "Edit the file to configure your
juju environment and re-run bootstrap.\n")
On 2013/02/04 15:49:16, fwereade wrote:
> s/re-run/run/

Done.

https://codereview.appspot.com/7223063/diff/7003/environs/boilerplate_config.go
File environs/boilerplate_config.go (right):

https://codereview.appspot.com/7223063/diff/7003/environs/boilerplate_config....
environs/boilerplate_config.go:32: panic(fmt.Sprintf("error from crypto rand:
%v", err))
On 2013/02/04 09:39:25, rog wrote:
> s/Sprintf/Errorf/
> 
> just convention - not that it makes much difference.

Done.

https://codereview.appspot.com/7223063/diff/7003/environs/boilerplate_config....
environs/boilerplate_config.go:51: indent(&config, ecfg.Bytes(), " ")
On 2013/02/04 09:39:25, rog wrote:
> i think we use two-space indent elsewhere, so probably good to use here too.

Done.

https://codereview.appspot.com/7223063/diff/7003/environs/boilerplate_config....
environs/boilerplate_config.go:68: // indent appends the given text to the given
buffer indented by the given ident string.
On 2013/02/04 09:39:25, rog wrote:
> s/ident/indent/

Done.

https://codereview.appspot.com/7223063/diff/7003/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/7223063/diff/7003/environs/dummy/environs.go#n...
environs/dummy/environs.go:390: dummy:
On 2013/02/04 09:39:25, rog wrote:
> we don't need to indent this here any more.

Done.

https://codereview.appspot.com/7223063/diff/7003/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/7223063/diff/7003/environs/ec2/ec2.go#newcode112
environs/ec2/ec2.go:112: amazon:
On 2013/02/04 09:39:25, rog wrote:
> no indent required.

Done.

https://codereview.appspot.com/7223063/diff/7003/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/7223063/diff/7003/environs/interface.go#newcode24
environs/interface.go:24: // The text should be have a key followed by some
number of attributes:
On 2013/02/04 09:39:25, rog wrote:
> s/be have/be/
> 
> oops

Done.

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

https://codereview.appspot.com/7223063/diff/7003/environs/openstack/config.go...
environs/openstack/config.go:17: "auth-mode":         schema.String(),
On 2013/02/04 09:39:25, rog wrote:
> i don't think this change should be here. but it may well be a codereview
> artifact - ignore me if this is a change that's already gone into trunk.

Python juju uses "auth-mode" and we want the configs to be compatible between Go
juju and Python juju. Since this branch generates boilerplate configs, I needed
to make the generated config correct. But then since I was doing that, it didn't
make sense not to change this also. So this change is more of a drive by to
ensure full consistency across the code base.

https://codereview.appspot.com/7223063/diff/7003/environs/openstack/provider.go
File environs/openstack/provider.go (right):

https://codereview.appspot.com/7223063/diff/7003/environs/openstack/provider....
environs/openstack/provider.go:64: openstack:
On 2013/02/04 09:39:25, rog wrote:
> we can lose the indent here.

Done.

[wallyworld, 2013-02-04 23:58:16]

*** Submitted:

Boilerplate environments.yaml

A new command "juju generate-config" is introduced to generate sensible config
for each registered provider. The Environs interface has a new method
"BoilerPlateConfig()"
which is used to gather the config for each provider.

If a user has an environments.yaml already, it is not overwritten. Otherwise the
boilerplate
has sensible defaults which just need a little tweaking to get a working system.

As new providers are rehistered (eg local, mass), the boilerplate header will
need to be tweaked
to refer to the new providers.

As a driveby, since config is being generated, I've renamed auth-method to
auth-mode. 
Python juju uses auth-mode and Go juju should be compatible.

R=dimitern, jameinel, fwereade, rog
CC=
https://codereview.appspot.com/7223063