uniter/jujuc: added arg --all to config-get

uniter/jujuc: added arg --all to config-get

Actually config-get behaves different to pyjuju and writes
all configuration keys, even if their value is nil. Now
the default behavior is like in pyjuju, empty values are
filtered. The additional argument --all (or -a) allows
the output of all keys.

https://code.launchpad.net/~themue/juju-core/029-config-get-all/+merge/170789

(do not edit description out of merge proposal)

[mue, 2013-06-21 11:22:35]

Please take a look.

[rog, 2013-06-21 12:08:50]

LGTM with a couple of minor suggestions

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get.go
File worker/uniter/jujuc/config-get.go (right):

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
worker/uniter/jujuc/config-get.go:35: f.BoolVar(&c.all, "a", false, "write also
keys without values")
"when no key given, print all keys, including those with no value" ?

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
File worker/uniter/jujuc/config-get_test.go (right):

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
worker/uniter/jujuc/config-get_test.go:45: {[]string{"--all", "--format",
"json"}, configGetJsonMapAll},
another test for "-a" ?

[fwereade, 2013-06-21 12:18:31]

Solid code, but a bunch of tweaks that I think would help a lot.

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get.go
File worker/uniter/jujuc/config-get.go (left):

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
worker/uniter/jujuc/config-get.go:40: c.Key = args[0]
I think Key and All should be mutually exclusive.

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get.go
File worker/uniter/jujuc/config-get.go (right):

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
worker/uniter/jujuc/config-get.go:16: all bool
Can we capitalise this? I think it's consistent...

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
worker/uniter/jujuc/config-get.go:29: Doc:     "If a key is given, only the
value for that key will be printed.",
Expand this to explain --all properly without space constraints.

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
worker/uniter/jujuc/config-get.go:35: f.BoolVar(&c.all, "a", false, "write also
keys without values")
On 2013/06/21 12:08:50, rog wrote:
> "when no key given, print all keys, including those with no value" ?

"print all keys"

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
File worker/uniter/jujuc/config-get_test.go (right):

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
worker/uniter/jujuc/config-get_test.go:25: configGetJsonMapAll =
`{"empty":null,"monsters":false,"spline-reticulation":45,"title":"My
Title","username":"admin001"}` + "\n"
I think I did this originally, but I think it's crack. To check yaml/json output
we should be unmarshalling the actual result, rather than depending on the yaml
marshalling picking a particular representation.

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
worker/uniter/jujuc/config-get_test.go:45: {[]string{"--all", "--format",
"json"}, configGetJsonMapAll},
On 2013/06/21 12:08:50, rog wrote:
> another test for "-a" ?

+1; also one for an error if both key and --all are specified.

[mue, 2013-06-21 15:19:51]

Please take a look.

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get.go
File worker/uniter/jujuc/config-get.go (left):

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
worker/uniter/jujuc/config-get.go:40: c.Key = args[0]
On 2013/06/21 12:18:31, fwereade wrote:
> I think Key and All should be mutually exclusive.

Done.

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get.go
File worker/uniter/jujuc/config-get.go (right):

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
worker/uniter/jujuc/config-get.go:16: all bool
On 2013/06/21 12:18:31, fwereade wrote:
> Can we capitalise this? I think it's consistent...

Done.

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
worker/uniter/jujuc/config-get.go:29: Doc:     "If a key is given, only the
value for that key will be printed.",
On 2013/06/21 12:18:31, fwereade wrote:
> Expand this to explain --all properly without space constraints.

Done. But not happy with the text, need feedback of a native speaker.

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
worker/uniter/jujuc/config-get.go:35: f.BoolVar(&c.all, "a", false, "write also
keys without values")
On 2013/06/21 12:18:31, fwereade wrote:
> On 2013/06/21 12:08:50, rog wrote:
> > "when no key given, print all keys, including those with no value" ?
> 
> "print all keys"

Done.

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
File worker/uniter/jujuc/config-get_test.go (right):

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
worker/uniter/jujuc/config-get_test.go:25: configGetJsonMapAll =
`{"empty":null,"monsters":false,"spline-reticulation":45,"title":"My
Title","username":"admin001"}` + "\n"
On 2013/06/21 12:18:31, fwereade wrote:
> I think I did this originally, but I think it's crack. To check yaml/json
output
> we should be unmarshalling the actual result, rather than depending on the
yaml
> marshalling picking a particular representation.

Done.

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get...
worker/uniter/jujuc/config-get_test.go:45: {[]string{"--all", "--format",
"json"}, configGetJsonMapAll},
On 2013/06/21 12:18:31, fwereade wrote:
> On 2013/06/21 12:08:50, rog wrote:
> > another test for "-a" ?
> 
> +1; also one for an error if both key and --all are specified.

Done.

[dpb1, 2013-06-24 23:02:47]

I tested this patch set 2 on trunk with the apache2 module from the charm store.
 It broke.  The values that were set to empty string in the config.yaml for the
charm ended up being set to null instead of empty string.

It does appear to address the extra values not showing up, however.

[mue, 2013-06-25 10:27:18]

On 2013/06/24 23:02:47, dpb1 wrote:
> I tested this patch set 2 on trunk with the apache2 module from the charm
store.
>  It broke.  The values that were set to empty string in the config.yaml for
the
> charm ended up being set to null instead of empty string.
> 
> It does appear to address the extra values not showing up, however.

Hi David,

this patch handles https://bugs.launchpad.net/juju-core/+bug/1192728, where we
so far showed all values, not only the nun-null. Here we had a difference to
pyjuju. Now it behaves like pyjuju in this point and you need the argument --all
to behave like gojuju without this patch.

Your behavior of null value even when set to an empty string in config.yaml
reminds me of https://bugs.launchpad.net/juju-core/+bug/1192706. Here we're
currently in discussion (see issue) about the old or a new but consistent
behavior.

mue

[fwereade, 2013-06-26 09:07:56]

LGTM

https://codereview.appspot.com/10441044/diff/8001/worker/uniter/jujuc/config-...
File worker/uniter/jujuc/config-get.go (right):

https://codereview.appspot.com/10441044/diff/8001/worker/uniter/jujuc/config-...
worker/uniter/jujuc/config-get.go:27: doc := `
When no <key> is supplied, all keys with values or defaults are printed. If
--all is set, all known keys are printed; those without defaults or values are
reported as null. <key> and --all are mutually exclusive.

[mue, 2013-06-26 09:36:02]

https://codereview.appspot.com/10441044/diff/8001/worker/uniter/jujuc/config-...
File worker/uniter/jujuc/config-get.go (right):

https://codereview.appspot.com/10441044/diff/8001/worker/uniter/jujuc/config-...
worker/uniter/jujuc/config-get.go:27: doc := `
On 2013/06/26 09:07:57, fwereade wrote:
> When no <key> is supplied, all keys with values or defaults are printed. If
> --all is set, all known keys are printed; those without defaults or values are
> reported as null. <key> and --all are mutually exclusive.

Done.