init will write the environments.yaml by default.

init will write the environments.yaml by default.

Changes the behaviour of the command so that:
 1. By default it will create the environments.yaml if it doesn't exist.
 2. If environments.yaml already exists, it errors appropriately (with hint about -f)
 3. If -f is provided, it will overwrite the existing environments.yaml
 4. If --show is used, it will instead print the boilerplate to stdout.

https://code.launchpad.net/~michael.nelson/juju-core/1208491-juju-init-w/+merge/178600

(do not edit description out of merge proposal)

[Michael Nelson, 2013-08-05 17:02:25]

Please take a look.

[dimitern, 2013-08-05 17:18:09]

Nice, LGTM, just a few suggestions.

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

https://codereview.appspot.com/12469044/diff/1/cmd/juju/init.go#newcode29
cmd/juju/init.go:29: f.BoolVar(&c.WriteFile, "w", false, "Overwrite existing
environments.yaml file.")

Since all descriptions are with first-lower-case, please use "overwrite ...".

https://codereview.appspot.com/12469044/diff/1/cmd/juju/init.go#newcode39
cmd/juju/init.go:39: return fmt.Errorf("A juju environment configuration already
exists.\nUse -w to overwrite the existing environments.yaml.\n")

Perhaps define a:

var errJujuEnvExists = fmt.Errorf(`A juju ...
Use -w to ...`)

just before this function and return it here.

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

https://codereview.appspot.com/12469044/diff/1/cmd/juju/init_test.go#newcode63
cmd/juju/init_test.go:63: // explicit.
s/explicit/given explicitly/. ?

[Michael Nelson, 2013-08-05 17:52:07]

Please take a look.

[dimitern, 2013-08-05 18:30:26]

Thanks! One more suggestion.

Please, next time reply to the inline comments here on codereview.appspot.com,
it's easier to track what was done and you can also give answers/ask questions
with that feature as well.

https://codereview.appspot.com/12469044/diff/7001/cmd/juju/init.go
File cmd/juju/init.go (right):

https://codereview.appspot.com/12469044/diff/7001/cmd/juju/init.go#newcode33
cmd/juju/init.go:33: "A juju environment configuration already exists.\n" +
My idea was to use ` quotes here, so you don't need to concatenate or add \n in
the string, so:

var errJujuEnvExists = fmt.Errorf(`A juju environment configuration already
exists.
Use -w to overwrite the existing environments.yaml.
`)

[Michael Nelson, 2013-08-05 18:42:53]

On Mon, Aug 5, 2013 at 7:30 PM, <dimiter.naydenov@canonical.com> wrote:

> Thanks! One more suggestion.
>
> Please, next time reply to the inline comments here on
> codereview.appspot.com, it's easier to track what was done and you can
> also give answers/ask questions with that feature as well.
>

Right - and it means I can be a bit friendlier and say thanks for the
review :)


>
>
https://codereview.appspot.**com/12469044/diff/7001/cmd/**juju/init.go<https:...
> File cmd/juju/init.go (right):
>
> https://codereview.appspot.**com/12469044/diff/7001/cmd/**
>
juju/init.go#newcode33<https://codereview.appspot.com/12469044/diff/7001/cmd/juju/init.go#newcode33>
> cmd/juju/init.go:33: "A juju environment configuration already
> exists.\n" +
> My idea was to use ` quotes here, so you don't need to concatenate or
> add \n in the string, so:
>
> var errJujuEnvExists = fmt.Errorf(`A juju environment configuration
> already exists.
>
> Use -w to overwrite the existing environments.yaml.
> `)
>
>
Ah - I see. I've done that with r1606 and pushed to LP.

Thanks!

[dimitern, 2013-08-05 18:51:11]

>
https://codereview.appspot.**com/12469044/diff/7001/cmd/**juju/init.go%3Chttp...>
> > File cmd/juju/init.go (right):
> >
> > https://codereview.appspot.**com/12469044/diff/7001/cmd/**
> >
>
juju/init.go#newcode33<https://codereview.appspot.com/12469044/diff/7001/cmd/juju/init.go#newcode33>
> > cmd/juju/init.go:33: "A juju environment configuration already
> > exists.\n" +
> > My idea was to use ` quotes here, so you don't need to concatenate or
> > add \n in the string, so:
> >
> > var errJujuEnvExists = fmt.Errorf(`A juju environment configuration
> > already exists.
> >
> > Use -w to overwrite the existing environments.yaml.
> > `)
> >
> >
> Ah - I see. I've done that with r1606 and pushed to LP.
> 
> Thanks!

No worries, you're welcome :)
In order to see the changes here on rietveld, it's necessary to do lbox propose
again, once you do the changes, rather than just bzr push to LP. It has the
extra nice feature of publishing any draft comments/replies you have entered, so
you don't have to manually do that. It also keeps both the LP MP and the CL on
rietveld in sync, which is nice, because most of us juju-core guys prefer to see
all changes here - inline comments are very useful, and probably the only nice
feature of rietveld otherwise. LP reviews would rock if it had them.

[Michael Nelson, 2013-08-05 20:47:02]

Please take a look.

[rog, 2013-08-06 09:06:27]

Looks good, but I've got a couple of suggestions
that you might want to consider.

https://codereview.appspot.com/12469044/diff/15001/cmd/juju/init.go
File cmd/juju/init.go (left):

https://codereview.appspot.com/12469044/diff/15001/cmd/juju/init.go#oldcode38
cmd/juju/init.go:38: fmt.Fprintln(out, config)
Personally, I will miss this functionality.
I think it's useful to be able to print the boilerplate
config to see default configuration for newly added
providers, for example. This CL means I cannot
do that in a straightforward way.

We could perhaps add a --print or --show (or ... ?) flag
that prints the config to stdout

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

https://codereview.appspot.com/12469044/diff/15001/cmd/juju/init.go#newcode29
cmd/juju/init.go:29: f.BoolVar(&c.WriteFile, "w", false, "overwrite existing
environments.yaml file.")
I think -f would be a more conventional spelling for a flag
with these semantics.

[Michael Nelson, 2013-08-06 10:24:06]

Please take a look.

https://codereview.appspot.com/12469044/diff/15001/cmd/juju/init.go
File cmd/juju/init.go (left):

https://codereview.appspot.com/12469044/diff/15001/cmd/juju/init.go#oldcode38
cmd/juju/init.go:38: fmt.Fprintln(out, config)
On 2013/08/06 09:06:27, rog wrote:
> Personally, I will miss this functionality.
> I think it's useful to be able to print the boilerplate
> config to see default configuration for newly added
> providers, for example. This CL means I cannot
> do that in a straightforward way.
> 
> We could perhaps add a --print or --show (or ... ?) flag
> that prints the config to stdout

I added --show which prints the new boilerplate before the instructions (so the
instructions don't get lost in the output).

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

https://codereview.appspot.com/12469044/diff/15001/cmd/juju/init.go#newcode29
cmd/juju/init.go:29: f.BoolVar(&c.WriteFile, "w", false, "overwrite existing
environments.yaml file.")
On 2013/08/06 09:06:27, rog wrote:
> I think -f would be a more conventional spelling for a flag
> with these semantics.

Done.

[rog, 2013-08-06 10:31:30]

https://codereview.appspot.com/12469044/diff/21001/cmd/juju/init.go
File cmd/juju/init.go (right):

https://codereview.appspot.com/12469044/diff/21001/cmd/juju/init.go#newcode30
cmd/juju/init.go:30: f.BoolVar(&c.WriteFile, "f", false, "overwrite existing
environments.yaml file.")
"force overwriting environments.yaml file even if it exists (ignored if --show
flag specified)"

?

https://codereview.appspot.com/12469044/diff/21001/cmd/juju/init.go#newcode31
cmd/juju/init.go:31: f.BoolVar(&c.ShowFile, "show", false, "show the existing
environments.yaml file.")
I think this should be:

f.BoolVar(&c.ShowFile, "show", false, "Print the generated configuration data to
stdout instead of writing it to a file")

with appropriate changes below.

[Michael Nelson, 2013-08-06 10:44:10]

Please take a look.

https://codereview.appspot.com/12469044/diff/21001/cmd/juju/init.go
File cmd/juju/init.go (right):

https://codereview.appspot.com/12469044/diff/21001/cmd/juju/init.go#newcode30
cmd/juju/init.go:30: f.BoolVar(&c.WriteFile, "f", false, "overwrite existing
environments.yaml file.")
On 2013/08/06 10:31:30, rog wrote:
> "force overwriting environments.yaml file even if it exists (ignored if --show
> flag specified)"
> 
> ?

Done.

https://codereview.appspot.com/12469044/diff/21001/cmd/juju/init.go#newcode31
cmd/juju/init.go:31: f.BoolVar(&c.ShowFile, "show", false, "show the existing
environments.yaml file.")
On 2013/08/06 10:31:30, rog wrote:
> I think this should be:
> 
> f.BoolVar(&c.ShowFile, "show", false, "Print the generated configuration data
to
> stdout instead of writing it to a file")
> 
> with appropriate changes below.

Done.

[rog, 2013-08-06 10:47:36]

LGTM with one tiny suggestion below.

https://codereview.appspot.com/12469044/diff/26001/cmd/juju/init.go
File cmd/juju/init.go (right):

https://codereview.appspot.com/12469044/diff/26001/cmd/juju/init.go#newcode18
cmd/juju/init.go:18: ShowFile  bool

Perhaps just "Show" might be more appropriate
given its current semantics?

[Michael Nelson, 2013-08-06 10:57:44]

Please take a look.

https://codereview.appspot.com/12469044/diff/26001/cmd/juju/init.go
File cmd/juju/init.go (right):

https://codereview.appspot.com/12469044/diff/26001/cmd/juju/init.go#newcode18
cmd/juju/init.go:18: ShowFile  bool
On 2013/08/06 10:47:36, rog wrote:
> 
> Perhaps just "Show" might be more appropriate
> given its current semantics?

Done.