juju-core

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

  1. fix-no-peers-charms
  2. Merge into trunk
Proposed by Jimmie Butler
Status: Needs review
Proposed branch: lp:~jimmiebtlr/juju-core/fix-no-peers-charms
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 198 lines (+52/-17)
5 files modified
charm/meta.go (+2/-2)
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-charms
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+221156@code.launchpad.net

Commit message

charm: accept missing relation maps

https://codereview.appspot.com/101810046/

Description of the change

Allow charms without a peer listed.

https://codereview.appspot.com/101810046/

To post a comment you must log in.
Revision history for this message
Jimmie Butler (jimmiebtlr) wrote :
Download full text (8.0 KiB)

Reviewers: mp+221156_code.launchpad.net,

Message:
Please take a look.

Description:
Allow charms without a peer listed.

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

(do not edit description out of merge proposal)

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

Affected files (+61, -24 lines):
   A [revision details]
   M charm/meta.go
   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-20140528050243-agcci9rl0vqrr911
+New revision: <email address hidden>

Index: charm/meta.go
=== modified file 'charm/meta.go'
--- charm/meta.go 2014-04-09 16:36:12 +0000
+++ charm/meta.go 2014-05-21 01:59:05 +0000
@@ -96,13 +96,13 @@
   Summary string
   Description string
   Subordinate bool
- Provides map[string]Relation `bson:",omitempty"`
- Requires map[string]Relation `bson:",omitempty"`
- Peers map[string]Relation `bson:",omitempty"`
- Format int `bson:",omitempty"`
- OldRevision int `bson:",omitempty"` // Obsolete
- Categories []string `bson:",omitempty"`
- Series string `bson:",omitempty"`
+ Provides map[string]Relation
+ Requires map[string]Relation
+ Peers map[string]Relation
+ Format int `bson:",omitempty"`
+ OldRevision int `bson:",omitempty"` // Obsolete
+ Categories []string `bson:",omitempty"`
+ Series string `bson:",omitempty"`
  }

  func generateRelationHooks(relName string, allHooks map[string]bool) {
@@ -262,10 +262,10 @@
  }

  func parseRelations(relations interface{}, role RelationRole)
map[string]Relation {
+ result := make(map[string]Relation)
   if relations == nil {
- return nil
+ return result
   }
- result := make(map[string]Relation)
   for name, rel := range relations.(map[string]interface{}) {
    relMap := rel.(map[string]interface{})
    relation := Relation{

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-28 05:01:55 +0000
@@ -93,8 +93,8 @@
    Interface: "mysql",
    Scope: charm.ScopeGlobal,
   })
- c.Assert(meta.Requires, gc.IsNil)
- c.Assert(meta.Peers, gc.IsNil)
+ c.Assert(meta.Requires, gc.DeepEquals, map[string]charm.Relation{})
+ c.Assert(meta.Peers, gc.DeepEquals, map[string]charm.Relation{})

   meta, err = charm.ReadMeta(repoMeta("riak"))
   c.Assert(err, gc.IsNil)
@@ -117,7 +117,7 @@
    Limit: 1,
    Scope: charm.ScopeGlobal,
   })
- c.Assert(meta.Requires, gc.IsNil)
+ c.Assert(meta.Requires, gc.DeepEquals, map[string]charm.Relation{})

   meta, err = charm.ReadMeta(repoMeta("terracotta"))
   c.Assert(err, gc.IsNil)
@@ -135,7 +135,7 @@
    Limit: 1,
    Scope: charm.ScopeGlobal,
   })
- c.Assert(meta.Requires, gc.IsNil)
+ c.Assert(meta.Requires, gc.DeepEquals, map[string]charm.Relation{})

   meta, err = charm.ReadMeta...

Read more...

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

LGTM apart from the serialisation change. What prompted that?

https://codereview.appspot.com/101810046/diff/1/charm/meta.go
File charm/meta.go (right):

https://codereview.appspot.com/101810046/diff/1/charm/meta.go#newcode101
charm/meta.go:101: Peers map[string]Relation
why did these change?

https://codereview.appspot.com/101810046/

Reply
Revision history for this message
Jimmie Butler (jimmiebtlr) wrote :

I'll change the serialization back, my thinking on that section was to be
more explicit. It does seem unnecessary though.
On May 28, 2014 3:57 AM, "William Reade" <email address hidden>
wrote:

> LGTM apart from the serialisation change. What prompted that?
>
>
> https://codereview.appspot.com/101810046/diff/1/charm/meta.go
> File charm/meta.go (right):
>
> https://codereview.appspot.com/101810046/diff/1/charm/meta.go#newcode101
> charm/meta.go:101: Peers map[string]Relation
> why did these change?
>
> https://codereview.appspot.com/101810046/
>
> --
>
> https://code.launchpad.net/~jimmiebtlr/juju-core/fix-no-peers-charms/+merge/221156
> You are the owner of lp:~jimmiebtlr/juju-core/fix-no-peers-charms.
>

Reply
Revision history for this message
Jimmie Butler (jimmiebtlr) wrote :

Please take a look.

https://codereview.appspot.com/101810046/

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

LGTM, I'm approving this despite the unnecessary lines in the test. A
followup that just dropped those would be very much appreciated.

https://codereview.appspot.com/101810046/diff/20001/charm/meta_test.go
File charm/meta_test.go (right):

https://codereview.appspot.com/101810046/diff/20001/charm/meta_test.go#newcode407
charm/meta_test.go:407: empty_input.Requires = nil
Aren't these nil already?

https://codereview.appspot.com/101810046/

Reply
Revision history for this message
Go Bot (go-bot) wrote :
Download full text (64.8 KiB)

The attempt to merge lp:~jimmiebtlr/juju-core/fix-no-peers-charms into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.024s
ok launchpad.net/juju-core/agent 0.923s
ok launchpad.net/juju-core/agent/mongo 0.484s
ok launchpad.net/juju-core/agent/tools 0.180s
ok launchpad.net/juju-core/audit 0.013s
ok launchpad.net/juju-core/bzr 5.142s
ok launchpad.net/juju-core/cert 2.623s
ok launchpad.net/juju-core/charm 0.414s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.174s
ok launchpad.net/juju-core/cloudinit/sshinit 0.765s
ok launchpad.net/juju-core/cmd 0.183s
ok launchpad.net/juju-core/cmd/charm-admin 0.244s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/envcmd 0.167s
ok launchpad.net/juju-core/cmd/juju 162.882s
ok launchpad.net/juju-core/cmd/jujud 69.584s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 9.346s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.151s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.023s
ok launchpad.net/juju-core/container 0.169s
ok launchpad.net/juju-core/container/factory 0.194s
ok launchpad.net/juju-core/container/kvm 0.166s
ok launchpad.net/juju-core/container/kvm/mock 0.162s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 4.270s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.193s
ok launchpad.net/juju-core/environs 2.002s
ok launchpad.net/juju-core/environs/bootstrap 3.981s
ok launchpad.net/juju-core/environs/cloudinit 0.518s
ok launchpad.net/juju-core/environs/config 1.605s
ok launchpad.net/juju-core/environs/configstore 0.177s
ok launchpad.net/juju-core/environs/filestorage 0.029s
ok launchpad.net/juju-core/environs/httpstorage 0.732s
ok launchpad.net/juju-core/environs/imagemetadata 0.519s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.216s
ok launchpad.net/juju-core/environs/jujutest 0.162s
ok launchpad.net/juju-core/environs/manual 11.665s
? launchpad.net/juju-core/environs/network [no test files]
ok launchpad.net/juju-core/environs/simplestreams 0.273s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.162s
ok launchpad.net/juju-core/environs/storage 0.787s
ok launchpad.net/juju-core/environs/sync 41.865s
ok launchpad.net/juju-core/environs/testing 0.146s
ok launchpad.net/juju-core/environs/tools 4.741s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/instance 0.145s
? launchpad.net/juju-core/instance/tes...

Reply
Revision history for this message
John A Meinel (jameinel) wrote :

So these appear to be genuine test discrepancies. At the least places that expected an empty map seem to have a nil map. I'm not 100% sure what the best fix is, possibly just fix up the tests to expect something different.

Reply
Revision history for this message
Horacio Durán (hduran-8) wrote :

On 2014/06/02 09:11:54, fwereade wrote:
> LGTM, I'm approving this despite the unnecessary lines in the test. A
 followup
> that just dropped those would be very much appreciated.

> https://codereview.appspot.com/101810046/diff/20001/charm/meta_test.go
> File charm/meta_test.go (right):

https://codereview.appspot.com/101810046/diff/20001/charm/meta_test.go#newcode407
> charm/meta_test.go:407: empty_input.Requires = nil
> Aren't these nil already?

LGTM Altough for the records I would appreciate a more detailed
description for the
merge proposal.

https://codereview.appspot.com/101810046/

Reply

Unmerged revisions

2717. By Jimmie Butler

Change test to pass with omitempty for relations.

2716. By Jimmie Butler

Merge in most recent master

2715. By Jimmie Butler

Remove serialization change

2714. By Jimmie Butler

Merge in most recent master

2713. By Jimmie Butler

Go fmt.

2712. By Jimmie Butler

Change test order, put error check first.

2711. By Jimmie Butler

Change test order to check error first.

2710. By Jimmie Butler

Merge with most recent master.

2709. By Jimmie Butler

Merge in most recent master

2708. By Jimmie Butler

Remove unneeded statement in parseRelation.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'charm/meta.go'
--- charm/meta.go 2014-04-09 16:36:12 +0000
+++ charm/meta.go 2014-05-29 01:51:08 +0000
@@ -262,10 +262,10 @@
262}262}
263263
264func parseRelations(relations interface{}, role RelationRole) map[string]Relation {264func parseRelations(relations interface{}, role RelationRole) map[string]Relation {
265 result := make(map[string]Relation)
265 if relations == nil {266 if relations == nil {
266 return nil267 return result
267 }268 }
268 result := make(map[string]Relation)
269 for name, rel := range relations.(map[string]interface{}) {269 for name, rel := range relations.(map[string]interface{}) {
270 relMap := rel.(map[string]interface{})270 relMap := rel.(map[string]interface{})
271 relation := Relation{271 relation := Relation{
272272
=== modified file 'charm/meta_test.go'
--- charm/meta_test.go 2014-04-09 16:36:12 +0000
+++ charm/meta_test.go 2014-05-29 01:51:08 +0000
@@ -93,8 +93,8 @@
93 Interface: "mysql",93 Interface: "mysql",
94 Scope: charm.ScopeGlobal,94 Scope: charm.ScopeGlobal,
95 })95 })
96 c.Assert(meta.Requires, gc.IsNil)96 c.Assert(meta.Requires, gc.DeepEquals, map[string]charm.Relation{})
97 c.Assert(meta.Peers, gc.IsNil)97 c.Assert(meta.Peers, gc.DeepEquals, map[string]charm.Relation{})
9898
99 meta, err = charm.ReadMeta(repoMeta("riak"))99 meta, err = charm.ReadMeta(repoMeta("riak"))
100 c.Assert(err, gc.IsNil)100 c.Assert(err, gc.IsNil)
@@ -117,7 +117,7 @@
117 Limit: 1,117 Limit: 1,
118 Scope: charm.ScopeGlobal,118 Scope: charm.ScopeGlobal,
119 })119 })
120 c.Assert(meta.Requires, gc.IsNil)120 c.Assert(meta.Requires, gc.DeepEquals, map[string]charm.Relation{})
121121
122 meta, err = charm.ReadMeta(repoMeta("terracotta"))122 meta, err = charm.ReadMeta(repoMeta("terracotta"))
123 c.Assert(err, gc.IsNil)123 c.Assert(err, gc.IsNil)
@@ -135,7 +135,7 @@
135 Limit: 1,135 Limit: 1,
136 Scope: charm.ScopeGlobal,136 Scope: charm.ScopeGlobal,
137 })137 })
138 c.Assert(meta.Requires, gc.IsNil)138 c.Assert(meta.Requires, gc.DeepEquals, map[string]charm.Relation{})
139139
140 meta, err = charm.ReadMeta(repoMeta("wordpress"))140 meta, err = charm.ReadMeta(repoMeta("wordpress"))
141 c.Assert(err, gc.IsNil)141 c.Assert(err, gc.IsNil)
@@ -160,7 +160,13 @@
160 Optional: true,160 Optional: true,
161 Scope: charm.ScopeGlobal,161 Scope: charm.ScopeGlobal,
162 })162 })
163 c.Assert(meta.Peers, gc.IsNil)163 c.Assert(meta.Peers, gc.DeepEquals, map[string]charm.Relation{})
164
165 meta, err = charm.ReadMeta(repoMeta("empty-fields"))
166 c.Assert(err, gc.IsNil)
167 c.Assert(meta.Provides, gc.DeepEquals, map[string]charm.Relation{})
168 c.Assert(meta.Requires, gc.DeepEquals, map[string]charm.Relation{})
169 c.Assert(meta.Peers, gc.DeepEquals, map[string]charm.Relation{})
164}170}
165171
166var relationsConstraintsTests = []struct {172var relationsConstraintsTests = []struct {
@@ -396,12 +402,15 @@
396 for i, codec := range codecs {402 for i, codec := range codecs {
397 c.Logf("codec %d", i)403 c.Logf("codec %d", i)
398 empty_input := charm.Meta{}404 empty_input := charm.Meta{}
405 empty_input.Provides = nil
406 empty_input.Peers = nil
407 empty_input.Requires = nil
399 data, err := codec.Marshal(empty_input)408 data, err := codec.Marshal(empty_input)
400 c.Assert(err, gc.IsNil)409 c.Assert(err, gc.IsNil)
401 var empty_output charm.Meta410 var empty_output charm.Meta
402 err = codec.Unmarshal(data, &empty_output)411 err = codec.Unmarshal(data, &empty_output)
403 c.Assert(err, gc.IsNil)412 c.Assert(err, gc.IsNil)
404 c.Assert(empty_input, gc.DeepEquals, empty_output)413 c.Assert(empty_output, gc.DeepEquals, empty_input)
405 }414 }
406}415}
407416
408417
=== modified file 'schema/schema.go'
--- schema/schema.go 2014-01-15 10:50:11 +0000
+++ schema/schema.go 2014-05-29 01:51:08 +0000
@@ -293,6 +293,11 @@
293293
294func (c mapC) Coerce(v interface{}, path []string) (interface{}, error) {294func (c mapC) Coerce(v interface{}, path []string) (interface{}, error) {
295 rv := reflect.ValueOf(v)295 rv := reflect.ValueOf(v)
296
297 // treat nil value as empty map
298 if rv.Kind() == reflect.Invalid {
299 return make(map[interface{}]interface{}), nil
300 }
296 if rv.Kind() != reflect.Map {301 if rv.Kind() != reflect.Map {
297 return nil, error_{"map", v, path}302 return nil, error_{"map", v, path}
298 }303 }
@@ -334,6 +339,11 @@
334339
335func (c stringMapC) Coerce(v interface{}, path []string) (interface{}, error) {340func (c stringMapC) Coerce(v interface{}, path []string) (interface{}, error) {
336 rv := reflect.ValueOf(v)341 rv := reflect.ValueOf(v)
342
343 // treat nil value as empty map
344 if rv.Kind() == reflect.Invalid {
345 return make(map[string]interface{}), nil
346 }
337 if rv.Kind() != reflect.Map {347 if rv.Kind() != reflect.Map {
338 return nil, error_{"map", v, path}348 return nil, error_{"map", v, path}
339 }349 }
@@ -399,6 +409,11 @@
399409
400func (c fieldMapC) Coerce(v interface{}, path []string) (interface{}, error) {410func (c fieldMapC) Coerce(v interface{}, path []string) (interface{}, error) {
401 rv := reflect.ValueOf(v)411 rv := reflect.ValueOf(v)
412
413 // treat nil value as empty map
414 if rv.Kind() == reflect.Invalid {
415 return make(map[string]interface{}), nil
416 }
402 if rv.Kind() != reflect.Map {417 if rv.Kind() != reflect.Map {
403 return nil, error_{"map", v, path}418 return nil, error_{"map", v, path}
404 }419 }
@@ -488,6 +503,11 @@
488503
489func (c mapSetC) Coerce(v interface{}, path []string) (interface{}, error) {504func (c mapSetC) Coerce(v interface{}, path []string) (interface{}, error) {
490 rv := reflect.ValueOf(v)505 rv := reflect.ValueOf(v)
506
507 // treat nil value as empty map
508 if rv.Kind() == reflect.Invalid {
509 return make(map[string]interface{}), nil
510 }
491 if rv.Kind() != reflect.Map {511 if rv.Kind() != reflect.Map {
492 return nil, error_{"map", v, path}512 return nil, error_{"map", v, path}
493 }513 }
494514
=== modified file 'schema/schema_test.go'
--- schema/schema_test.go 2014-01-14 13:10:26 +0000
+++ schema/schema_test.go 2014-05-29 01:51:08 +0000
@@ -250,8 +250,8 @@
250 c.Assert(err, gc.ErrorMatches, "<path>: expected map, got int\\(42\\)")250 c.Assert(err, gc.ErrorMatches, "<path>: expected map, got int\\(42\\)")
251251
252 out, err = sch.Coerce(nil, aPath)252 out, err = sch.Coerce(nil, aPath)
253 c.Assert(out, gc.IsNil)253 c.Assert(err, gc.IsNil)
254 c.Assert(err, gc.ErrorMatches, "<path>: expected map, got nothing")254 c.Assert(out, gc.DeepEquals, map[interface{}]interface{}{})
255255
256 out, err = sch.Coerce(map[int]int{1: 1}, aPath)256 out, err = sch.Coerce(map[int]int{1: 1}, aPath)
257 c.Assert(out, gc.IsNil)257 c.Assert(out, gc.IsNil)
@@ -274,12 +274,12 @@
274 c.Assert(out, gc.DeepEquals, map[string]interface{}{"a": int64(1), "b": int64(2)})274 c.Assert(out, gc.DeepEquals, map[string]interface{}{"a": int64(1), "b": int64(2)})
275275
276 out, err = sch.Coerce(42, aPath)276 out, err = sch.Coerce(42, aPath)
277 c.Assert(out, gc.IsNil)
278 c.Assert(err, gc.ErrorMatches, "<path>: expected map, got int\\(42\\)")277 c.Assert(err, gc.ErrorMatches, "<path>: expected map, got int\\(42\\)")
278 c.Assert(out, gc.IsNil)
279279
280 out, err = sch.Coerce(nil, aPath)280 out, err = sch.Coerce(nil, aPath)
281 c.Assert(out, gc.IsNil)281 c.Assert(err, gc.IsNil)
282 c.Assert(err, gc.ErrorMatches, "<path>: expected map, got nothing")282 c.Assert(out, gc.DeepEquals, map[string]interface{}{})
283283
284 out, err = sch.Coerce(map[int]int{1: 1}, aPath)284 out, err = sch.Coerce(map[int]int{1: 1}, aPath)
285 c.Assert(out, gc.IsNil)285 c.Assert(out, gc.IsNil)
@@ -306,8 +306,8 @@
306 c.Assert(err, gc.ErrorMatches, "<path>: expected map, got int\\(42\\)")306 c.Assert(err, gc.ErrorMatches, "<path>: expected map, got int\\(42\\)")
307307
308 out, err = sch.Coerce(nil, aPath)308 out, err = sch.Coerce(nil, aPath)
309 c.Assert(out, gc.IsNil)309 c.Assert(err, gc.IsNil)
310 c.Assert(err, gc.ErrorMatches, "<path>: expected map, got nothing")310 c.Assert(out, gc.DeepEquals, map[string]interface{}{})
311311
312 out, err = sch.Coerce(map[string]interface{}{"a": "A", "b": "C"}, aPath)312 out, err = sch.Coerce(map[string]interface{}{"a": "A", "b": "C"}, aPath)
313 c.Assert(out, gc.IsNil)313 c.Assert(out, gc.IsNil)
@@ -416,8 +416,8 @@
416 c.Assert(err, gc.ErrorMatches, `<path>: expected map, got int\(42\)`)416 c.Assert(err, gc.ErrorMatches, `<path>: expected map, got int\(42\)`)
417417
418 out, err = sch.Coerce(nil, aPath)418 out, err = sch.Coerce(nil, aPath)
419 c.Assert(out, gc.IsNil)419 c.Assert(err, gc.IsNil)
420 c.Assert(err, gc.ErrorMatches, `<path>: expected map, got nothing`)420 c.Assert(out, gc.DeepEquals, map[string]interface{}{})
421421
422 // First path entry shouldn't have dots in an error message.422 // First path entry shouldn't have dots in an error message.
423 out, err = sch.Coerce(map[string]int{"a": 1}, nil)423 out, err = sch.Coerce(map[string]int{"a": 1}, nil)
424424
=== added directory 'testing/repo/quantal/empty-fields'
=== added file 'testing/repo/quantal/empty-fields/metadata.yaml'
--- testing/repo/quantal/empty-fields/metadata.yaml 1970-01-01 00:00:00 +0000
+++ testing/repo/quantal/empty-fields/metadata.yaml 2014-05-29 01:51:08 +0000
@@ -0,0 +1,6 @@
1name: empty-fields
2summary: "Kerosene powered blender"
3description: "Description here"
4provides:
5peers:
6requires:

Subscribers

People subscribed via source and target branches

to status/vote changes: