Merge lp://staging/~thumper/juju-core/get-environment into lp://staging/~juju/juju-core/trunk

Reviewers: mp+157968_code.launchpad.net,

Message:
Please take a look.

Description:
Add a get-environment command.

Be able to get environment values from state. You can ask for either a
single
value, or get the whole lot formatted how you like (well within reason).

set-environment coming real soon now.

https://code.launchpad.net/~thumper/juju-core/get-environment/+merge/157968

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/8597045/

Affected files:
   A [revision details]
   M cmd/cmd.go
   A cmd/juju/environment.go
   A cmd/juju/environment_test.go
   M cmd/juju/main.go
   M cmd/juju/main_test.go

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

https://codereview.appspot.com/8597045/diff/1/cmd/juju/main_test.go#newcode219
cmd/juju/main_test.go:219: "get-env", // alias for set-environment
Fixed the alias comment in the branch already, but didn't want to push
up a new change set.

https://codereview.appspot.com/8597045/

Looks good, but I have some questions.

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

https://codereview.appspot.com/8597045/diff/1/cmd/juju/environment.go#newcode13
cmd/juju/environment.go:13: type GetEnvironmentCommand struct {
Why not just juju get instead?

https://codereview.appspot.com/8597045/diff/1/cmd/juju/environment.go#newcode21
cmd/juju/environment.go:21: for the environment are output using the
selected formatter.
Is the formatter handled by the base command? I cannot see it
specifically formatting the output here.

https://codereview.appspot.com/8597045/diff/1/cmd/juju/environment.go#newcode75
cmd/juju/environment.go:75: return fmt.Errorf("Environment key %q not
found in %q environment.", c.key, config.Name())
s/Environment//

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

https://codereview.appspot.com/8597045/diff/1/cmd/juju/environment_test.go#newcode33
cmd/juju/environment_test.go:33: },
how about a test with all values? maybe even one with different formats?

https://codereview.appspot.com/8597045/

After speaking with fwereade, I drop my comment about the name.

On 2013/04/10 09:56:11, dimitern wrote:
> Looks good, but I have some questions.

> https://codereview.appspot.com/8597045/diff/1/cmd/juju/environment.go
> File cmd/juju/environment.go (right):

https://codereview.appspot.com/8597045/diff/1/cmd/juju/environment.go#newcode13
> cmd/juju/environment.go:13: type GetEnvironmentCommand struct {
> Why not just juju get instead?

https://codereview.appspot.com/8597045/

LGTM modulo ZeroOrOneArgs suggestion and more-detailed testing.

https://codereview.appspot.com/8597045/diff/1/cmd/cmd.go
File cmd/cmd.go (right):

https://codereview.appspot.com/8597045/diff/1/cmd/cmd.go#newcode49
cmd/cmd.go:49: func (c *CommandBase) ZeroOrOneArgs(args []string)
(string, error) {
I don't really think this deserves to live on the type. Free func like
CheckEmpty?

https://codereview.appspot.com/8597045/diff/1/cmd/cmd.go#newcode54
cmd/cmd.go:54: return result, CheckEmpty(args)
I generally prefer to return definite zero values with errors.

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

https://codereview.appspot.com/8597045/diff/1/cmd/juju/environment.go#newcode13
cmd/juju/environment.go:13: type GetEnvironmentCommand struct {
On 2013/04/10 09:56:11, dimitern wrote:
> Why not just juju get instead?

Different realms, potentially different options and formatting. Better
to keep them separate, as agreed in Atlanta.

https://codereview.appspot.com/8597045/diff/1/cmd/juju/environment.go#newcode21
cmd/juju/environment.go:21: for the environment are output using the
selected formatter.
On 2013/04/10 09:56:11, dimitern wrote:
> Is the formatter handled by the base command? I cannot see it
specifically
> formatting the output here.

c.out.AddFlags, c.out.Write

https://codereview.appspot.com/8597045/diff/1/cmd/juju/environment.go#newcode43
cmd/juju/environment.go:43: // TODO(thumper) --private to also include
private.
"to exclude private settings when unset" or something, just to be
explicit?

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

https://codereview.appspot.com/8597045/diff/1/cmd/juju/environment_test.go#newcode33
cmd/juju/environment_test.go:33: },
On 2013/04/10 09:56:11, dimitern wrote:
> how about a test with all values? maybe even one with different
formats?

+1 for all values, +0 for multiple formats -- the flag should be tested,
but we don't need to test its behaviour exhaustively.

https://codereview.appspot.com/8597045/

Updated the branch, but I'm going to land this at the same time as
set-environment so we get the pair together.

https://codereview.appspot.com/8597045/diff/1/cmd/cmd.go
File cmd/cmd.go (right):

https://codereview.appspot.com/8597045/diff/1/cmd/cmd.go#newcode49
cmd/cmd.go:49: func (c *CommandBase) ZeroOrOneArgs(args []string)
(string, error) {
On 2013/04/10 11:33:25, fwereade wrote:
> I don't really think this deserves to live on the type. Free func like
> CheckEmpty?

Fair enough.

https://codereview.appspot.com/8597045/diff/1/cmd/cmd.go#newcode54
cmd/cmd.go:54: return result, CheckEmpty(args)
On 2013/04/10 11:33:25, fwereade wrote:
> I generally prefer to return definite zero values with errors.

Done.

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

https://codereview.appspot.com/8597045/diff/1/cmd/juju/environment.go#newcode43
cmd/juju/environment.go:43: // TODO(thumper) --private to also include
private.
On 2013/04/10 11:33:25, fwereade wrote:
> "to exclude private settings when unset" or something, just to be
explicit?

This was initially a consideration about whether to include private
values by default, and that we could have an option to include them. In
the end I just implemented the "get everything" option. We can always
add a --private back in if desired at a later time. I'll just remove
the comment.

https://codereview.appspot.com/8597045/diff/1/cmd/juju/environment.go#newcode75
cmd/juju/environment.go:75: return fmt.Errorf("Environment key %q not
found in %q environment.", c.key, config.Name())
On 2013/04/10 09:56:11, dimitern wrote:
> s/Environment//

Done.

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

https://codereview.appspot.com/8597045/diff/1/cmd/juju/environment_test.go#newcode33
cmd/juju/environment_test.go:33: },
On 2013/04/10 11:33:25, fwereade wrote:
> On 2013/04/10 09:56:11, dimitern wrote:
> > how about a test with all values? maybe even one with different
formats?

> +1 for all values, +0 for multiple formats -- the flag should be
tested, but we
> don't need to test its behaviour exhaustively.

The all values is covered below in TestAllValues.

I deliberately kept these tests separate, as indicated by
TestSingleValue :)

https://codereview.appspot.com/8597045/

*** Submitted:

Add a get-environment command.

Be able to get environment values from state. You can ask for either a
single
value, or get the whole lot formatted how you like (well within reason).

set-environment coming next.

R=dfc, dimitern, fwereade
CC=
https://codereview.appspot.com/8597045

https://codereview.appspot.com/8597045/