Merge lp:~jimmiebtlr/juju-core/fix-no-peers-charm into lp:~go-bot/juju-core/trunk

Proposed by Jimmie Butler on 2014-05-03
Status: Rejected
Rejected by: William Reade on 2014-05-28
Proposed branch: lp:~jimmiebtlr/juju-core/fix-no-peers-charm
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 219 lines (+59/-24)
5 files modified
charm/meta.go (+9/-9)
charm/meta_test.go (+15/-6)
schema/schema.go (+20/-0)
schema/schema_test.go (+9/-9)
testing/repo/quantal/empty-fields/metadata.yaml (+6/-0)
To merge this branch: bzr merge lp:~jimmiebtlr/juju-core/fix-no-peers-charm
Reviewer Review Type Date Requested Status
Juju Engineering 2014-05-03 Pending
Review via email: mp+218195@code.launchpad.net

Description of the change

Allow charm with empty peers, provides, requires.

In the case that a null was read in for the maps, use
an empty map instead of throwing error. Added test case
to show error. Bug #1313793

https://codereview.appspot.com/98410043/

To post a comment you must log in.
John A Meinel (jameinel) wrote :

31 @@ -353,6 +359,7 @@
32 vpath[len(vpath)-1] = fmt.Sprint(k.Interface())
33 newv, err := c.value.Coerce(rv.MapIndex(k).Interface(), vpath)
34 if err != nil {
35 +
36 return nil, err
37 }
38 out[newk.(string)] = newv

I think this is a spurious change.

I'm a bit surprised we don't have a direct test of schema/* that would fit this change. I believe you have it covered, but only at the charm level.
Is there a test in schema that you could add that would be a bit more of a direct test?

Otherwise, LGTM.

Jimmie Butler (jimmiebtlr) wrote :

I'll take a look at the schema/* tests, and see about removing the
extraneous space. Thanks for the feedback.

On Mon, May 5, 2014 at 9:23 PM, John A Meinel <email address hidden>wrote:

> 31 @@ -353,6 +359,7 @@
> 32 vpath[len(vpath)-1] = fmt.Sprint(k.Interface())
> 33 newv, err := c.value.Coerce(rv.MapIndex(k).Interface(), vpath)
> 34 if err != nil {
> 35 +
> 36 return nil, err
> 37 }
> 38 out[newk.(string)] = newv
>
> I think this is a spurious change.
>
> I'm a bit surprised we don't have a direct test of schema/* that would fit
> this change. I believe you have it covered, but only at the charm level.
> Is there a test in schema that you could add that would be a bit more of a
> direct test?
>
> Otherwise, LGTM.
> --
>
> https://code.launchpad.net/~jimmiebtlr/juju-core/fix-no-peers-charm/+merge/218195
> You are the owner of lp:~jimmiebtlr/juju-core/fix-no-peers-charm.
>

--
Thanks,
Jimmie Butler

William Reade (fwereade) wrote :

FWIW, if you're planning to fix schema stringMap to assume nil->empty-map (+1 to that approach) please make sure the other map types in schema are consistent.

William Reade (fwereade) wrote :

Thanks, this was definitely the right place to make the change. A few more comments:

10 + c.Assert(err, gc.IsNil)

Please check the content of the returned metadata, on general principles. If we end up with inconsistent sometimes-nil-sometimes-empty behaviour, please fix that too; a casual reading indicates that we won't but some tests might start failing because we now have empty maps where we expected nils.

24 + return make(map[string]interface{}), nil

mapC should be returning a map[interface{}]interface{}

35 + // handles case where map is empty
48 + // handles case where map is empty
61 + // handles case where map is empty

"treat nil value as empty map", perhaps? And, FWIW, I'd skip the blank lines around the stanzas you inserted, but YMMV legitimately.

Jimmie Butler (jimmiebtlr) wrote :
Download full text (5.1 KiB)

Reviewers: mp+218195_code.launchpad.net,

Message:
Please take a look.

Description:
Allow charm with empty peers, provides, requires.

In the case that a null was read in for the maps, use
an empty map instead of throwing error. Added test case
to show error. Bug #1313793

https://code.launchpad.net/~jimmiebtlr/juju-core/fix-no-peers-charm/+merge/218195

(do not edit description out of merge proposal)

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

Affected files (+42, -8 lines):
   A [revision details]
   M charm/meta_test.go
   M schema/schema.go
   M schema/schema_test.go
   A testing/repo/quantal/empty-fields/metadata.yaml

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-20140520024339-qids5ewddm0js4yq
+New revision: <email address hidden>

Index: charm/meta_test.go
=== modified file 'charm/meta_test.go'
--- charm/meta_test.go 2014-04-09 16:36:12 +0000
+++ charm/meta_test.go 2014-05-20 03:20:37 +0000
@@ -161,6 +161,12 @@
    Scope: charm.ScopeGlobal,
   })
   c.Assert(meta.Peers, gc.IsNil)
+
+ meta, err = charm.ReadMeta(repoMeta("empty-fields"))
+ c.Assert(meta.Provides, gc.DeepEquals, map[string]charm.Relation{})
+ c.Assert(meta.Requires, gc.DeepEquals, map[string]charm.Relation{})
+ c.Assert(meta.Peers, gc.DeepEquals, map[string]charm.Relation{})
+ c.Assert(err, gc.IsNil)
  }

  var relationsConstraintsTests = []struct {

Index: schema/schema.go
=== modified file 'schema/schema.go'
--- schema/schema.go 2014-01-15 10:50:11 +0000
+++ schema/schema.go 2014-05-20 03:20:37 +0000
@@ -293,6 +293,11 @@

  func (c mapC) Coerce(v interface{}, path []string) (interface{}, error) {
   rv := reflect.ValueOf(v)
+
+ // handles case where map is empty
+ if rv.Kind() == reflect.Invalid {
+ return make(map[interface{}]interface{}), nil
+ }
   if rv.Kind() != reflect.Map {
    return nil, error_{"map", v, path}
   }
@@ -334,6 +339,11 @@

  func (c stringMapC) Coerce(v interface{}, path []string) (interface{},
error) {
   rv := reflect.ValueOf(v)
+
+ // treat nil value as empty map
+ if rv.Kind() == reflect.Invalid {
+ return make(map[string]interface{}), nil
+ }
   if rv.Kind() != reflect.Map {
    return nil, error_{"map", v, path}
   }
@@ -399,6 +409,11 @@

  func (c fieldMapC) Coerce(v interface{}, path []string) (interface{},
error) {
   rv := reflect.ValueOf(v)
+
+ // treat nil value as empty map
+ if rv.Kind() == reflect.Invalid {
+ return make(map[string]interface{}), nil
+ }
   if rv.Kind() != reflect.Map {
    return nil, error_{"map", v, path}
   }
@@ -488,6 +503,11 @@

  func (c mapSetC) Coerce(v interface{}, path []string) (interface{}, error)
{
   rv := reflect.ValueOf(v)
+
+ // treat nil value as empty map
+ if rv.Kind() == reflect.Invalid {
+ return make(map[string]interface{}), nil
+ }
   if rv.Kind() != reflect.Map {
    return nil, error_{"map", v, path}
   }

Index: schema/schema_test.go
=== modified file 'schema/schema_test.go'
--- schema/schema_test.go 2014-01-14 13:10:26 +0000
+++ schema/schema_test.go 2014-05-20 03:20:3...

Read more...

Dave Cheney (dave-cheney) wrote :

On 2014/05/20 03:33:38, jimmiebtlr wrote:
> Please take a look.

This code looks fine but I don't feel I can +1 this change without some
discussion as this changes the behaviour of _all_ map, mapSet, etc
fields.

https://codereview.appspot.com/98410043/

William Reade (fwereade) wrote :

LGTM, I'd accept this as-is, but please take a little look at the
nil/map{} issue. If it's too complex, let me know, and I'll approve it.

Dave, thanks for the caution; but schema isn't as heavily used as one
might think (especially the map bits), and I think this is a good
change.

https://codereview.appspot.com/98410043/diff/1/charm/meta_test.go
File charm/meta_test.go (left):

https://codereview.appspot.com/98410043/diff/1/charm/meta_test.go#oldcode163
charm/meta_test.go:163: c.Assert(meta.Peers, gc.IsNil)
It's this bit that bugs me, that I alluded to yesterday -- having a Meta
type that sometimes has nil and sometimes has {} feels weirdly
inconsistent. It's not really a very big deal, but I'd appreciate a fix
if it's not excessively complex to change.

https://codereview.appspot.com/98410043/diff/1/schema/schema.go
File schema/schema.go (right):

https://codereview.appspot.com/98410043/diff/1/schema/schema.go#newcode297
schema/schema.go:297: // handles case where map is empty
treat nil value as empty map :)

https://codereview.appspot.com/98410043/

Jimmie Butler (jimmiebtlr) wrote :

Ok, misunderstood that little bit. Basically it should return an empty map
rather than nil even if peers, requires, and provides are not mentioned in
the charm file. That will require a change in a different location,
meaning we may not need to change the map behavior at all.

On Tue, May 20, 2014 at 12:48 AM, William Reade <<email address hidden>
> wrote:

> LGTM, I'd accept this as-is, but please take a little look at the
> nil/map{} issue. If it's too complex, let me know, and I'll approve it.
>
> Dave, thanks for the caution; but schema isn't as heavily used as one
> might think (especially the map bits), and I think this is a good
> change.
>
>
> https://codereview.appspot.com/98410043/diff/1/charm/meta_test.go
> File charm/meta_test.go (left):
>
>
> https://codereview.appspot.com/98410043/diff/1/charm/meta_test.go#oldcode163
> charm/meta_test.go:163: c.Assert(meta.Peers, gc.IsNil)
> It's this bit that bugs me, that I alluded to yesterday -- having a Meta
> type that sometimes has nil and sometimes has {} feels weirdly
> inconsistent. It's not really a very big deal, but I'd appreciate a fix
> if it's not excessively complex to change.
>
> https://codereview.appspot.com/98410043/diff/1/schema/schema.go
> File schema/schema.go (right):
>
> https://codereview.appspot.com/98410043/diff/1/schema/schema.go#newcode297
> schema/schema.go:297: // handles case where map is empty
> treat nil value as empty map :)
>
> https://codereview.appspot.com/98410043/
>
> --
>
> https://code.launchpad.net/~jimmiebtlr/juju-core/fix-no-peers-charm/+merge/218195
> You are the owner of lp:~jimmiebtlr/juju-core/fix-no-peers-charm.
>

--
Thanks,
Jimmie Butler

Jimmie Butler (jimmiebtlr) wrote :
William Reade (fwereade) wrote :

> Ok, misunderstood that little bit. Basically it should return an empty map
> rather than nil even if peers, requires, and provides are not mentioned in
> the charm file. That will require a change in a different location,
> meaning we may not need to change the map behavior at all.

An easy alternative would be to just return nil maps (of the right type) from the schema package. I'm not bothered about which we pick so much as I am about consistency.

William Reade (fwereade) wrote :
Jimmie Butler (jimmiebtlr) wrote :

Correct, thanks
On May 28, 2014 3:57 AM, "William Reade" <email address hidden>
wrote:

> The proposal to merge lp:~jimmiebtlr/juju-core/fix-no-peers-charm into
> lp:juju-core has been updated.
>
> Status: Needs review => Rejected
>
> For more details, see:
>
> https://code.launchpad.net/~jimmiebtlr/juju-core/fix-no-peers-charm/+merge/218195
> --
>
> https://code.launchpad.net/~jimmiebtlr/juju-core/fix-no-peers-charm/+merge/218195
> You are the owner of lp:~jimmiebtlr/juju-core/fix-no-peers-charm.
>

Unmerged revisions

2713. By Jimmie Butler on 2014-05-28

Go fmt.

2712. By Jimmie Butler on 2014-05-28

Change test order, put error check first.

2711. By Jimmie Butler on 2014-05-28

Change test order to check error first.

2710. By Jimmie Butler on 2014-05-28

Merge with most recent master.

2709. By Jimmie Butler on 2014-05-26

Merge in most recent master

2708. By Jimmie Butler on 2014-05-21

Remove unneeded statement in parseRelation.

2707. By Jimmie Butler on 2014-05-21

Merge master chanegs

2706. By Jimmie Butler on 2014-05-21

Add back commented empty charm fields test.

2705. By Jimmie Butler on 2014-05-21

Go fmt.

2704. By Jimmie Butler on 2014-05-21

Merge in most recent master

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'charm/meta.go'
2--- charm/meta.go 2014-04-09 16:36:12 +0000
3+++ charm/meta.go 2014-05-28 05:03:23 +0000
4@@ -96,13 +96,13 @@
5 Summary string
6 Description string
7 Subordinate bool
8- Provides map[string]Relation `bson:",omitempty"`
9- Requires map[string]Relation `bson:",omitempty"`
10- Peers map[string]Relation `bson:",omitempty"`
11- Format int `bson:",omitempty"`
12- OldRevision int `bson:",omitempty"` // Obsolete
13- Categories []string `bson:",omitempty"`
14- Series string `bson:",omitempty"`
15+ Provides map[string]Relation
16+ Requires map[string]Relation
17+ Peers map[string]Relation
18+ Format int `bson:",omitempty"`
19+ OldRevision int `bson:",omitempty"` // Obsolete
20+ Categories []string `bson:",omitempty"`
21+ Series string `bson:",omitempty"`
22 }
23
24 func generateRelationHooks(relName string, allHooks map[string]bool) {
25@@ -262,10 +262,10 @@
26 }
27
28 func parseRelations(relations interface{}, role RelationRole) map[string]Relation {
29+ result := make(map[string]Relation)
30 if relations == nil {
31- return nil
32+ return result
33 }
34- result := make(map[string]Relation)
35 for name, rel := range relations.(map[string]interface{}) {
36 relMap := rel.(map[string]interface{})
37 relation := Relation{
38
39=== modified file 'charm/meta_test.go'
40--- charm/meta_test.go 2014-04-09 16:36:12 +0000
41+++ charm/meta_test.go 2014-05-28 05:03:23 +0000
42@@ -93,8 +93,8 @@
43 Interface: "mysql",
44 Scope: charm.ScopeGlobal,
45 })
46- c.Assert(meta.Requires, gc.IsNil)
47- c.Assert(meta.Peers, gc.IsNil)
48+ c.Assert(meta.Requires, gc.DeepEquals, map[string]charm.Relation{})
49+ c.Assert(meta.Peers, gc.DeepEquals, map[string]charm.Relation{})
50
51 meta, err = charm.ReadMeta(repoMeta("riak"))
52 c.Assert(err, gc.IsNil)
53@@ -117,7 +117,7 @@
54 Limit: 1,
55 Scope: charm.ScopeGlobal,
56 })
57- c.Assert(meta.Requires, gc.IsNil)
58+ c.Assert(meta.Requires, gc.DeepEquals, map[string]charm.Relation{})
59
60 meta, err = charm.ReadMeta(repoMeta("terracotta"))
61 c.Assert(err, gc.IsNil)
62@@ -135,7 +135,7 @@
63 Limit: 1,
64 Scope: charm.ScopeGlobal,
65 })
66- c.Assert(meta.Requires, gc.IsNil)
67+ c.Assert(meta.Requires, gc.DeepEquals, map[string]charm.Relation{})
68
69 meta, err = charm.ReadMeta(repoMeta("wordpress"))
70 c.Assert(err, gc.IsNil)
71@@ -160,7 +160,13 @@
72 Optional: true,
73 Scope: charm.ScopeGlobal,
74 })
75- c.Assert(meta.Peers, gc.IsNil)
76+ c.Assert(meta.Peers, gc.DeepEquals, map[string]charm.Relation{})
77+
78+ meta, err = charm.ReadMeta(repoMeta("empty-fields"))
79+ c.Assert(err, gc.IsNil)
80+ c.Assert(meta.Provides, gc.DeepEquals, map[string]charm.Relation{})
81+ c.Assert(meta.Requires, gc.DeepEquals, map[string]charm.Relation{})
82+ c.Assert(meta.Peers, gc.DeepEquals, map[string]charm.Relation{})
83 }
84
85 var relationsConstraintsTests = []struct {
86@@ -396,12 +402,15 @@
87 for i, codec := range codecs {
88 c.Logf("codec %d", i)
89 empty_input := charm.Meta{}
90+ empty_input.Provides = make(map[string]charm.Relation)
91+ empty_input.Peers = make(map[string]charm.Relation)
92+ empty_input.Requires = make(map[string]charm.Relation)
93 data, err := codec.Marshal(empty_input)
94 c.Assert(err, gc.IsNil)
95 var empty_output charm.Meta
96 err = codec.Unmarshal(data, &empty_output)
97 c.Assert(err, gc.IsNil)
98- c.Assert(empty_input, gc.DeepEquals, empty_output)
99+ c.Assert(empty_output, gc.DeepEquals, empty_input)
100 }
101 }
102
103
104=== modified file 'schema/schema.go'
105--- schema/schema.go 2014-01-15 10:50:11 +0000
106+++ schema/schema.go 2014-05-28 05:03:23 +0000
107@@ -293,6 +293,11 @@
108
109 func (c mapC) Coerce(v interface{}, path []string) (interface{}, error) {
110 rv := reflect.ValueOf(v)
111+
112+ // treat nil value as empty map
113+ if rv.Kind() == reflect.Invalid {
114+ return make(map[interface{}]interface{}), nil
115+ }
116 if rv.Kind() != reflect.Map {
117 return nil, error_{"map", v, path}
118 }
119@@ -334,6 +339,11 @@
120
121 func (c stringMapC) Coerce(v interface{}, path []string) (interface{}, error) {
122 rv := reflect.ValueOf(v)
123+
124+ // treat nil value as empty map
125+ if rv.Kind() == reflect.Invalid {
126+ return make(map[string]interface{}), nil
127+ }
128 if rv.Kind() != reflect.Map {
129 return nil, error_{"map", v, path}
130 }
131@@ -399,6 +409,11 @@
132
133 func (c fieldMapC) Coerce(v interface{}, path []string) (interface{}, error) {
134 rv := reflect.ValueOf(v)
135+
136+ // treat nil value as empty map
137+ if rv.Kind() == reflect.Invalid {
138+ return make(map[string]interface{}), nil
139+ }
140 if rv.Kind() != reflect.Map {
141 return nil, error_{"map", v, path}
142 }
143@@ -488,6 +503,11 @@
144
145 func (c mapSetC) Coerce(v interface{}, path []string) (interface{}, error) {
146 rv := reflect.ValueOf(v)
147+
148+ // treat nil value as empty map
149+ if rv.Kind() == reflect.Invalid {
150+ return make(map[string]interface{}), nil
151+ }
152 if rv.Kind() != reflect.Map {
153 return nil, error_{"map", v, path}
154 }
155
156=== modified file 'schema/schema_test.go'
157--- schema/schema_test.go 2014-01-14 13:10:26 +0000
158+++ schema/schema_test.go 2014-05-28 05:03:23 +0000
159@@ -250,8 +250,8 @@
160 c.Assert(err, gc.ErrorMatches, "<path>: expected map, got int\\(42\\)")
161
162 out, err = sch.Coerce(nil, aPath)
163- c.Assert(out, gc.IsNil)
164- c.Assert(err, gc.ErrorMatches, "<path>: expected map, got nothing")
165+ c.Assert(err, gc.IsNil)
166+ c.Assert(out, gc.DeepEquals, map[interface{}]interface{}{})
167
168 out, err = sch.Coerce(map[int]int{1: 1}, aPath)
169 c.Assert(out, gc.IsNil)
170@@ -274,12 +274,12 @@
171 c.Assert(out, gc.DeepEquals, map[string]interface{}{"a": int64(1), "b": int64(2)})
172
173 out, err = sch.Coerce(42, aPath)
174- c.Assert(out, gc.IsNil)
175 c.Assert(err, gc.ErrorMatches, "<path>: expected map, got int\\(42\\)")
176+ c.Assert(out, gc.IsNil)
177
178 out, err = sch.Coerce(nil, aPath)
179- c.Assert(out, gc.IsNil)
180- c.Assert(err, gc.ErrorMatches, "<path>: expected map, got nothing")
181+ c.Assert(err, gc.IsNil)
182+ c.Assert(out, gc.DeepEquals, map[string]interface{}{})
183
184 out, err = sch.Coerce(map[int]int{1: 1}, aPath)
185 c.Assert(out, gc.IsNil)
186@@ -306,8 +306,8 @@
187 c.Assert(err, gc.ErrorMatches, "<path>: expected map, got int\\(42\\)")
188
189 out, err = sch.Coerce(nil, aPath)
190- c.Assert(out, gc.IsNil)
191- c.Assert(err, gc.ErrorMatches, "<path>: expected map, got nothing")
192+ c.Assert(err, gc.IsNil)
193+ c.Assert(out, gc.DeepEquals, map[string]interface{}{})
194
195 out, err = sch.Coerce(map[string]interface{}{"a": "A", "b": "C"}, aPath)
196 c.Assert(out, gc.IsNil)
197@@ -416,8 +416,8 @@
198 c.Assert(err, gc.ErrorMatches, `<path>: expected map, got int\(42\)`)
199
200 out, err = sch.Coerce(nil, aPath)
201- c.Assert(out, gc.IsNil)
202- c.Assert(err, gc.ErrorMatches, `<path>: expected map, got nothing`)
203+ c.Assert(err, gc.IsNil)
204+ c.Assert(out, gc.DeepEquals, map[string]interface{}{})
205
206 // First path entry shouldn't have dots in an error message.
207 out, err = sch.Coerce(map[string]int{"a": 1}, nil)
208
209=== added directory 'testing/repo/quantal/empty-fields'
210=== added file 'testing/repo/quantal/empty-fields/metadata.yaml'
211--- testing/repo/quantal/empty-fields/metadata.yaml 1970-01-01 00:00:00 +0000
212+++ testing/repo/quantal/empty-fields/metadata.yaml 2014-05-28 05:03:23 +0000
213@@ -0,0 +1,6 @@
214+name: empty-fields
215+summary: "Kerosene powered blender"
216+description: "Description here"
217+provides:
218+peers:
219+requires:

Subscribers

People subscribed via source and target branches

to status/vote changes: