juju-core

Merge lp:~themue/juju-core/029-config-get-all into lp:~go-bot/juju-core/trunk

  1. 029-config-get-all
  2. Merge into trunk
Proposed by Frank Mueller
Status: Merged
Approved by: Frank Mueller
Approved revision: no longer in the source branch.
Merged at revision: 1332
Proposed branch: lp:~themue/juju-core/029-config-get-all
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 250 lines (+119/-11)
3 files modified
worker/uniter/jujuc/config-get.go (+22/-1)
worker/uniter/jujuc/config-get_test.go (+92/-10)
worker/uniter/jujuc/util_test.go (+5/-0)
To merge this branch: bzr merge lp:~themue/juju-core/029-config-get-all
Reviewer Review Type Date Requested Status
William Reade (community) Approve
Review via email: mp+170789@code.launchpad.net

Commit message

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://codereview.appspot.com/10441044/

Description of the change

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://codereview.appspot.com/10441044/

To post a comment you must log in.
Revision history for this message
Frank Mueller (themue) wrote :
Download full text (4.1 KiB)

Reviewers: mp+170789_code.launchpad.net,

Message:
Please take a look.

Description:
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)

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

Affected files:
   A [revision details]
   M worker/uniter/jujuc/config-get.go
   M worker/uniter/jujuc/config-get_test.go
   M worker/uniter/jujuc/util_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20130620172218-jwl9sp71dmuqty4r
+New revision: <email address hidden>

Index: worker/uniter/jujuc/config-get.go
=== modified file 'worker/uniter/jujuc/config-get.go'
--- worker/uniter/jujuc/config-get.go 2013-06-10 21:04:37 +0000
+++ worker/uniter/jujuc/config-get.go 2013-06-21 11:16:43 +0000
@@ -13,6 +13,7 @@
   cmd.CommandBase
   ctx Context
   Key string // The key to show. If empty, show all.
+ all bool
   out cmd.Output
  }

@@ -31,6 +32,8 @@

  func (c *ConfigGetCommand) SetFlags(f *gnuflag.FlagSet) {
   c.out.AddFlags(f, "smart", cmd.DefaultFormatters)
+ f.BoolVar(&c.all, "a", false, "write also keys without values")
+ f.BoolVar(&c.all, "all", false, "")
  }

  func (c *ConfigGetCommand) Init(args []string) error {
@@ -38,6 +41,7 @@
    return nil
   }
   c.Key = args[0]
+
   return cmd.CheckEmpty(args[1:])
  }

@@ -48,6 +52,13 @@
   }
   var value interface{}
   if c.Key == "" {
+ if !c.all {
+ for k, v := range settings {
+ if v == nil {
+ delete(settings, k)
+ }
+ }
+ }
    value = settings
   } else {
    value, _ = settings[c.Key]

Index: worker/uniter/jujuc/config-get_test.go
=== modified file 'worker/uniter/jujuc/config-get_test.go'
--- worker/uniter/jujuc/config-get_test.go 2013-05-02 15:55:42 +0000
+++ worker/uniter/jujuc/config-get_test.go 2013-06-21 11:16:43 +0000
@@ -18,7 +18,12 @@

  var _ = Suite(&ConfigGetSuite{})

-var configGetYamlMap = "monsters: false\nspline-reticulation: 45\ntitle:
My Title\nusername: admin001\n"
+var (
+ configGetYamlMap = "monsters: false\nspline-reticulation: 45\ntitle:
My Title\nusername: admin001\n"
+ configGetYamlMapAll = "empty: null\nmonsters: false\nspline-reticulation:
45\ntitle: My Title\nusername: admin001\n"
+ configGetJsonMap =
`{"monsters":false,"spline-reticulation":45,"title":"My
Title","username":"admin001"}` + "\n"
+ configGetJsonMapAll =
`{"empty":null,"monsters":false,"spline-reticulation":45,"title":"My
Title","username":"admin001"}` + "\n"
+)

  var configGetTests = []struct {
   args []string
@@ -35,7 +40,9 @@
   {[]string{"--format", "json", "missing"}, "null\n"},
   {nil, configGetYamlMap},
   {[]string{"--format", "yaml"}, configGetYamlMap},
- {[]string{"--format", "json"},
`{"mo...

Read more...

Revision history for this message
Roger Peppe (rogpeppe) wrote :

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.go#newcode35
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_test.go
File worker/uniter/jujuc/config-get_test.go (right):

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

https://codereview.appspot.com/10441044/

Revision history for this message
William Reade (fwereade) wrote :

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.go#oldcode40
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.go#newcode16
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.go#newcode29
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.go#newcode35
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_test.go
File worker/uniter/jujuc/config-get_test.go (right):

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get_test.go#newcode25
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_test.go#newcode45
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.

https://codereview.appspot.com/10441044/

Revision history for this message
Frank Mueller (themue) wrote :

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.go#oldcode40
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.go#newcode16
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.go#newcode29
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.go#newcode35
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_test.go
File worker/uniter/jujuc/config-get_test.go (right):

https://codereview.appspot.com/10441044/diff/1/worker/uniter/jujuc/config-get_test.go#newcode25
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_test.go#newcode45
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.

https://codereview.appspot.com/10441044/

Revision history for this message
David Britton (dpb) 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.

https://codereview.appspot.com/10441044/

Revision history for this message
Frank Mueller (themue) wrote :

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

https://codereview.appspot.com/10441044/

Revision history for this message
William Reade (fwereade) wrote :

LGTM

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

https://codereview.appspot.com/10441044/diff/8001/worker/uniter/jujuc/config-get.go#newcode27
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.

https://codereview.appspot.com/10441044/

Revision history for this message
William Reade (fwereade) :
review: Approve
Revision history for this message
William Reade (fwereade) :
review: Approve
Revision history for this message
Frank Mueller (themue) wrote :

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

https://codereview.appspot.com/10441044/diff/8001/worker/uniter/jujuc/config-get.go#newcode27
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.

https://codereview.appspot.com/10441044/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'worker/uniter/jujuc/config-get.go'
2--- worker/uniter/jujuc/config-get.go 2013-06-10 21:04:37 +0000
3+++ worker/uniter/jujuc/config-get.go 2013-06-26 09:37:33 +0000
4@@ -4,6 +4,8 @@
5 package jujuc
6
7 import (
8+ "fmt"
9+
10 "launchpad.net/gnuflag"
11 "launchpad.net/juju-core/cmd"
12 )
13@@ -13,6 +15,7 @@
14 cmd.CommandBase
15 ctx Context
16 Key string // The key to show. If empty, show all.
17+ All bool
18 out cmd.Output
19 }
20
21@@ -21,16 +24,23 @@
22 }
23
24 func (c *ConfigGetCommand) Info() *cmd.Info {
25+ doc := `
26+When no <key> is supplied, all keys with values or defaults are printed. If
27+--all is set, all known keys are printed; those without defaults or values are
28+reported as null. <key> and --all are mutually exclusive.
29+`
30 return &cmd.Info{
31 Name: "config-get",
32 Args: "[<key>]",
33 Purpose: "print service configuration",
34- Doc: "If a key is given, only the value for that key will be printed.",
35+ Doc: doc,
36 }
37 }
38
39 func (c *ConfigGetCommand) SetFlags(f *gnuflag.FlagSet) {
40 c.out.AddFlags(f, "smart", cmd.DefaultFormatters)
41+ f.BoolVar(&c.All, "a", false, "print all keys")
42+ f.BoolVar(&c.All, "all", false, "")
43 }
44
45 func (c *ConfigGetCommand) Init(args []string) error {
46@@ -38,6 +48,10 @@
47 return nil
48 }
49 c.Key = args[0]
50+ if c.Key != "" && c.All {
51+ return fmt.Errorf("cannot use argument --all together with key %q", c.Key)
52+ }
53+
54 return cmd.CheckEmpty(args[1:])
55 }
56
57@@ -48,6 +62,13 @@
58 }
59 var value interface{}
60 if c.Key == "" {
61+ if !c.All {
62+ for k, v := range settings {
63+ if v == nil {
64+ delete(settings, k)
65+ }
66+ }
67+ }
68 value = settings
69 } else {
70 value, _ = settings[c.Key]
71
72=== modified file 'worker/uniter/jujuc/config-get_test.go'
73--- worker/uniter/jujuc/config-get_test.go 2013-05-02 15:55:42 +0000
74+++ worker/uniter/jujuc/config-get_test.go 2013-06-26 09:37:33 +0000
75@@ -4,12 +4,15 @@
76 package jujuc_test
77
78 import (
79+ "encoding/json"
80 "io/ioutil"
81+ "path/filepath"
82+
83 . "launchpad.net/gocheck"
84+ "launchpad.net/goyaml"
85 "launchpad.net/juju-core/cmd"
86 "launchpad.net/juju-core/testing"
87 "launchpad.net/juju-core/worker/uniter/jujuc"
88- "path/filepath"
89 )
90
91 type ConfigGetSuite struct {
92@@ -18,9 +21,7 @@
93
94 var _ = Suite(&ConfigGetSuite{})
95
96-var configGetYamlMap = "monsters: false\nspline-reticulation: 45\ntitle: My Title\nusername: admin001\n"
97-
98-var configGetTests = []struct {
99+var configGetKeyTests = []struct {
100 args []string
101 out string
102 }{
103@@ -33,13 +34,10 @@
104 {[]string{"missing"}, ""},
105 {[]string{"--format", "yaml", "missing"}, ""},
106 {[]string{"--format", "json", "missing"}, "null\n"},
107- {nil, configGetYamlMap},
108- {[]string{"--format", "yaml"}, configGetYamlMap},
109- {[]string{"--format", "json"}, `{"monsters":false,"spline-reticulation":45,"title":"My Title","username":"admin001"}` + "\n"},
110 }
111
112-func (s *ConfigGetSuite) TestOutputFormat(c *C) {
113- for i, t := range configGetTests {
114+func (s *ConfigGetSuite) TestOutputFormatKey(c *C) {
115+ for i, t := range configGetKeyTests {
116 c.Logf("test %d: %#v", i, t.args)
117 hctx := s.GetHookContext(c, -1, "")
118 com, err := jujuc.NewCommand(hctx, "config-get")
119@@ -52,6 +50,76 @@
120 }
121 }
122
123+var (
124+ configGetYamlMap = map[string]interface{}{
125+ "monsters": false,
126+ "spline-reticulation": 45,
127+ "title": "My Title",
128+ "username": "admin001",
129+ }
130+ configGetJsonMap = map[string]interface{}{
131+ "monsters": false,
132+ "spline-reticulation": 45.0,
133+ "title": "My Title",
134+ "username": "admin001",
135+ }
136+ configGetYamlMapAll = map[string]interface{}{
137+ "empty": nil,
138+ "monsters": false,
139+ "spline-reticulation": 45,
140+ "title": "My Title",
141+ "username": "admin001",
142+ }
143+ configGetJsonMapAll = map[string]interface{}{
144+ "empty": nil,
145+ "monsters": false,
146+ "spline-reticulation": 45.0,
147+ "title": "My Title",
148+ "username": "admin001",
149+ }
150+)
151+
152+const (
153+ formatYaml = iota
154+ formatJson
155+)
156+
157+var configGetAllTests = []struct {
158+ args []string
159+ format int
160+ out map[string]interface{}
161+}{
162+ {nil, formatYaml, configGetYamlMap},
163+ {[]string{"--format", "yaml"}, formatYaml, configGetYamlMap},
164+ {[]string{"--format", "json"}, formatJson, configGetJsonMap},
165+ {[]string{"--all", "--format", "yaml"}, formatYaml, configGetYamlMapAll},
166+ {[]string{"--all", "--format", "json"}, formatJson, configGetJsonMapAll},
167+ {[]string{"-a", "--format", "yaml"}, formatYaml, configGetYamlMapAll},
168+ {[]string{"-a", "--format", "json"}, formatJson, configGetJsonMapAll},
169+}
170+
171+func (s *ConfigGetSuite) TestOutputFormatAll(c *C) {
172+ for i, t := range configGetAllTests {
173+ c.Logf("test %d: %#v", i, t.args)
174+ hctx := s.GetHookContext(c, -1, "")
175+ com, err := jujuc.NewCommand(hctx, "config-get")
176+ c.Assert(err, IsNil)
177+ ctx := testing.Context(c)
178+ code := cmd.Main(com, ctx, t.args)
179+ c.Assert(code, Equals, 0)
180+ c.Assert(bufferString(ctx.Stderr), Equals, "")
181+
182+ out := map[string]interface{}{}
183+ switch t.format {
184+ case formatYaml:
185+ c.Assert(goyaml.Unmarshal(bufferBytes(ctx.Stdout), &out), IsNil)
186+ case formatJson:
187+ c.Assert(json.Unmarshal(bufferBytes(ctx.Stdout), &out), IsNil)
188+ }
189+ c.Assert(out, DeepEquals, t.out)
190+ }
191+}
192+
193 func (s *ConfigGetSuite) TestHelp(c *C) {
194 hctx := s.GetHookContext(c, -1, "")
195 com, err := jujuc.NewCommand(hctx, "config-get")
196@@ -63,12 +131,16 @@
197 purpose: print service configuration
198
199 options:
200+-a, --all (= false)
201+ print all keys
202 --format (= smart)
203 specify output format (json|smart|yaml)
204 -o, --output (= "")
205 specify an output file
206
207-If a key is given, only the value for that key will be printed.
208+When no <key> is supplied, all keys with values or defaults are printed. If
209+--all is set, all known keys are printed; those without defaults or values are
210+reported as null. <key> and --all are mutually exclusive.
211 `)
212 c.Assert(bufferString(ctx.Stderr), Equals, "")
213 }
214@@ -93,3 +165,13 @@
215 c.Assert(err, IsNil)
216 testing.TestInit(c, com, []string{"multiple", "keys"}, `unrecognized args: \["keys"\]`)
217 }
218+
219+func (s *ConfigGetSuite) TestAllPlusKey(c *C) {
220+ hctx := s.GetHookContext(c, -1, "")
221+ com, err := jujuc.NewCommand(hctx, "config-get")
222+ c.Assert(err, IsNil)
223+ ctx := testing.Context(c)
224+ code := cmd.Main(com, ctx, []string{"--all", "--format", "json", "monsters"})
225+ c.Assert(code, Equals, 2)
226+ c.Assert(bufferString(ctx.Stderr), Equals, "error: cannot use argument --all together with key \"monsters\"\n")
227+}
228
229=== modified file 'worker/uniter/jujuc/util_test.go'
230--- worker/uniter/jujuc/util_test.go 2013-06-10 21:04:37 +0000
231+++ worker/uniter/jujuc/util_test.go 2013-06-26 09:37:33 +0000
232@@ -18,6 +18,10 @@
233
234 func TestPackage(t *testing.T) { TestingT(t) }
235
236+func bufferBytes(stream io.Writer) []byte {
237+ return stream.(*bytes.Buffer).Bytes()
238+}
239+
240 func bufferString(w io.Writer) string {
241 return w.(*bytes.Buffer).String()
242 }
243@@ -99,6 +103,7 @@
244
245 func (c *Context) ConfigSettings() (charm.Settings, error) {
246 return charm.Settings{
247+ "empty": nil,
248 "monsters": false,
249 "spline-reticulation": 45.0,
250 "title": "My Title",

Subscribers

People subscribed via source and target branches

to status/vote changes: