juju-core

Merge lp:~themue/juju-core/041-revert-empty-strings-in-charm-config into lp:~go-bot/juju-core/trunk

  1. 041-revert-empty-strings-in-charm-config
  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: 1725
Proposed branch: lp:~themue/juju-core/041-revert-empty-strings-in-charm-config
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 136 lines (+29/-16)
4 files modified
charm/config.go (+5/-0)
charm/config_test.go (+14/-13)
state/apiserver/client/get_test.go (+6/-3)
state/service_test.go (+4/-0)
To merge this branch: bzr merge lp:~themue/juju-core/041-revert-empty-strings-in-charm-config
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+182693@code.launchpad.net

Commit message

charm: reverted empty string branch

Reverted the branch allowing empty string values
in charm configurations due incompatability with
the API.

https://codereview.appspot.com/13349043/

Description of the change

charm: reverted empty string branch

Reverted the branch allowing empty string values
in charm configurations due incompatability with
the API.

https://codereview.appspot.com/13349043/

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

Reviewers: mp+182693_code.launchpad.net,

Message:
Please take a look.

Description:
charm: reverted empty string branch

Reverted the branch allowing empty string values
in charm configurations due incompatability with
the API.

https://code.launchpad.net/~themue/juju-core/041-revert-empty-strings-in-charm-config/+merge/182693

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M charm/config.go
   M charm/config_test.go
   M state/apiserver/client/get_test.go
   M state/service_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-20130828123627-r38fc3kmubd7nucc
+New revision: <email address hidden>

Index: charm/config.go
=== modified file 'charm/config.go'
--- charm/config.go 2013-08-26 15:39:55 +0000
+++ charm/config.go 2013-08-28 16:21:50 +0000
@@ -45,6 +45,8 @@
   if checker := optionTypeCheckers[option.Type]; checker != nil {
    if value, err = checker.Coerce(value, nil); err != nil {
     return nil, err
+ } else if value == "" {
+ value = nil
    }
    return value, nil
   }
@@ -62,6 +64,9 @@
  // returns an error if it cannot be parsed to the correct type. Empty
  // string values are returned as nil.
  func (option Option) parse(name, str string) (_ interface{}, err error) {
+ if str == "" {
+ return nil, nil
+ }
   defer option.error(&err, name, str)
   switch option.Type {
   case "string":

Index: charm/config_test.go
=== modified file 'charm/config_test.go'
--- charm/config_test.go 2013-08-26 15:39:55 +0000
+++ charm/config_test.go 2013-08-28 16:21:50 +0000
@@ -112,7 +112,7 @@
   c.Assert(settings, gc.DeepEquals, charm.Settings{
    "title": "something valid",
    "username": nil,
- "outlook": "",
+ "outlook": nil,
   })
  }

@@ -149,9 +149,9 @@
     "reticulate-splines": true,
    },
   }, {
- info: "empty string-typed values stay empty",
+ info: "empty string-typed values become nil",
    input: charm.Settings{"outlook": ""},
- expect: charm.Settings{"outlook": ""},
+ expect: charm.Settings{"outlook": nil},
   }, {
    info: "almost-correctly-typed values are valid",
    input: charm.Settings{
@@ -279,14 +279,14 @@
    key: "blah",
    expect: settingsWithNils,
   }, {
- info: "empty strings for non string options are not accepted",
+ info: "empty strings are considered nil",
    yaml: `blah:
              outlook: ""
              skill-level: ""
              agility-ratio: ""
              reticulate-splines: ""`,
- key: "blah",
- err: `option "skill-level" expected int, got ""`,
+ key: "blah",
+ expect: settingsWithNils,
   }, {
    info: "appropriate strings are valid",
    yaml: `blah:
@@ -331,13 +331,14 @@
    input: map[string]string{},
    expect: charm.Settings{},
   }, {
- info: "empty strings for string options are valid",
- input: map[string]string{"outlook": ""},
- expect: charm.Settings{"outlook": ""},
- }, {
- info: "empty strings for non string options are invalid",
- input...

Read more...

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

Given it is just reverting a patch you and William agreed should be
pulled out,
LGTM

https://codereview.appspot.com/13349043/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'charm/config.go'
--- charm/config.go 2013-08-26 15:39:55 +0000
+++ charm/config.go 2013-08-28 16:26:36 +0000
@@ -45,6 +45,8 @@
45 if checker := optionTypeCheckers[option.Type]; checker != nil {45 if checker := optionTypeCheckers[option.Type]; checker != nil {
46 if value, err = checker.Coerce(value, nil); err != nil {46 if value, err = checker.Coerce(value, nil); err != nil {
47 return nil, err47 return nil, err
48 } else if value == "" {
49 value = nil
48 }50 }
49 return value, nil51 return value, nil
50 }52 }
@@ -62,6 +64,9 @@
62// returns an error if it cannot be parsed to the correct type. Empty64// returns an error if it cannot be parsed to the correct type. Empty
63// string values are returned as nil.65// string values are returned as nil.
64func (option Option) parse(name, str string) (_ interface{}, err error) {66func (option Option) parse(name, str string) (_ interface{}, err error) {
67 if str == "" {
68 return nil, nil
69 }
65 defer option.error(&err, name, str)70 defer option.error(&err, name, str)
66 switch option.Type {71 switch option.Type {
67 case "string":72 case "string":
6873
=== modified file 'charm/config_test.go'
--- charm/config_test.go 2013-08-26 15:39:55 +0000
+++ charm/config_test.go 2013-08-28 16:26:36 +0000
@@ -112,7 +112,7 @@
112 c.Assert(settings, gc.DeepEquals, charm.Settings{112 c.Assert(settings, gc.DeepEquals, charm.Settings{
113 "title": "something valid",113 "title": "something valid",
114 "username": nil,114 "username": nil,
115 "outlook": "",115 "outlook": nil,
116 })116 })
117}117}
118118
@@ -149,9 +149,9 @@
149 "reticulate-splines": true,149 "reticulate-splines": true,
150 },150 },
151 }, {151 }, {
152 info: "empty string-typed values stay empty",152 info: "empty string-typed values become nil",
153 input: charm.Settings{"outlook": ""},153 input: charm.Settings{"outlook": ""},
154 expect: charm.Settings{"outlook": ""},154 expect: charm.Settings{"outlook": nil},
155 }, {155 }, {
156 info: "almost-correctly-typed values are valid",156 info: "almost-correctly-typed values are valid",
157 input: charm.Settings{157 input: charm.Settings{
@@ -279,14 +279,14 @@
279 key: "blah",279 key: "blah",
280 expect: settingsWithNils,280 expect: settingsWithNils,
281 }, {281 }, {
282 info: "empty strings for non string options are not accepted",282 info: "empty strings are considered nil",
283 yaml: `blah:283 yaml: `blah:
284 outlook: ""284 outlook: ""
285 skill-level: ""285 skill-level: ""
286 agility-ratio: ""286 agility-ratio: ""
287 reticulate-splines: ""`,287 reticulate-splines: ""`,
288 key: "blah",288 key: "blah",
289 err: `option "skill-level" expected int, got ""`,289 expect: settingsWithNils,
290 }, {290 }, {
291 info: "appropriate strings are valid",291 info: "appropriate strings are valid",
292 yaml: `blah:292 yaml: `blah:
@@ -331,13 +331,14 @@
331 input: map[string]string{},331 input: map[string]string{},
332 expect: charm.Settings{},332 expect: charm.Settings{},
333 }, {333 }, {
334 info: "empty strings for string options are valid",334 info: "empty strings are nil values",
335 input: map[string]string{"outlook": ""},335 input: map[string]string{
336 expect: charm.Settings{"outlook": ""},336 "outlook": "",
337 }, {337 "skill-level": "",
338 info: "empty strings for non string options are invalid",338 "agility-ratio": "",
339 input: map[string]string{"skill-level": ""},339 "reticulate-splines": "",
340 err: `option "skill-level" expected int, got ""`,340 },
341 expect: settingsWithNils,
341 }, {342 }, {
342 info: "strings are converted",343 info: "strings are converted",
343 input: map[string]string{344 input: map[string]string{
344345
=== modified file 'state/apiserver/client/get_test.go'
--- state/apiserver/client/get_test.go 2013-08-28 11:20:26 +0000
+++ state/apiserver/client/get_test.go 2013-08-28 16:26:36 +0000
@@ -57,7 +57,9 @@
57 "title": "Look To Windward",57 "title": "Look To Windward",
58 // Same as default.58 // Same as default.
59 "username": "admin001",59 "username": "admin001",
60 // Outlook and skill-level are left unset.60 // Use default (but there's no charm default)
61 "skill-level": "",
62 // Outlook is left unset.
61 },63 },
62 expect: params.ServiceGetResults{64 expect: params.ServiceGetResults{
63 Config: map[string]interface{}{65 Config: map[string]interface{}{
@@ -89,7 +91,7 @@
89 about: "deployed service #2",91 about: "deployed service #2",
90 charm: "dummy",92 charm: "dummy",
91 config: map[string]string{93 config: map[string]string{
92 // Empty string sets empty string94 // Empty string gives default
93 "title": "",95 "title": "",
94 // Value when there's a default96 // Value when there's a default
95 "username": "foobie",97 "username": "foobie",
@@ -103,7 +105,8 @@
103 "title": map[string]interface{}{105 "title": map[string]interface{}{
104 "description": "A descriptive title used for the service.",106 "description": "A descriptive title used for the service.",
105 "type": "string",107 "type": "string",
106 "value": "",108 "value": "My Title",
109 "default": true,
107 },110 },
108 "outlook": map[string]interface{}{111 "outlook": map[string]interface{}{
109 "description": "No default outlook.",112 "description": "No default outlook.",
110113
=== modified file 'state/service_test.go'
--- state/service_test.go 2013-08-27 14:28:19 +0000
+++ state/service_test.go 2013-08-28 16:26:36 +0000
@@ -344,6 +344,10 @@
344 about: "unset missing string",344 about: "unset missing string",
345 update: charm.Settings{"outlook": nil},345 update: charm.Settings{"outlook": nil},
346}, {346}, {
347 about: `empty strings unset string values`,
348 initial: charm.Settings{"outlook": "positive"},
349 update: charm.Settings{"outlook": "", "title": ""},
350}, {
347 about: "preserve existing value",351 about: "preserve existing value",
348 initial: charm.Settings{"title": "sir"},352 initial: charm.Settings{"title": "sir"},
349 update: charm.Settings{"username": "admin001"},353 update: charm.Settings{"username": "admin001"},

Subscribers

People subscribed via source and target branches

to status/vote changes: