charm: allow empty string options

charm: allow empty string options

The is the second CL for http://pad.lv/1194945, this time
keeping the behavior of the API. So now if empty strings
are passed as settings via Go it leads to empty string
options or an error in case of other types. Passing
empty strings via the API deletes them like before.

https://code.launchpad.net/~themue/juju-core/042-empty-values/+merge/183853

(do not edit description out of merge proposal)

[mue, 2013-09-04 11:23:34]

Please take a look.

[mue, 2013-09-04 11:31:15]

Please take a look.

[fwereade, 2013-09-04 15:48:08]

Looking solid, just a couple of comments

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/clie...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/clie...
state/apiserver/client/client.go:261: err = esc.validateAndMerge(ch, changes)
This block looks like it's repeated -- can we wrap the whole lot in a func?

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/get_...
File state/apiserver/client/get_test.go (right):

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/get_...
state/apiserver/client/get_test.go:93: config: map[string]string{
Aren't we just using the wrong type here? If we set the *actual* config values
we'll have a better test, I think.

[dimitern, 2013-09-05 07:18:46]

A few comments. I also agree with William's as well.

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/clie...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/clie...
state/apiserver/client/client.go:150: // Split settings due to compatability
reasons.
s/compatability/compatibility/

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/clie...
state/apiserver/client/client.go:155: // due to compatability.
ditto

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/clie...
state/apiserver/client/client.go:254: // Split settings due to compatability
reasons.
ditto

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/clie...
state/apiserver/client/client.go:260: // Validate and merge the unset settings,
again due to compatability.
ditto

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/clie...
state/apiserver/client/client.go:261: err = esc.validateAndMerge(ch, changes)
On 2013/09/04 15:48:09, fwereade wrote:
> This block looks like it's repeated -- can we wrap the whole lot in a func?
+1

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/clie...
state/apiserver/client/client.go:419: // emptySettingCompatability is a helper
type to keep the API behavior while the
Please change s/compatability/compatibility/ across the whole file.

[mue, 2013-09-05 11:32:25]

Please take a look.

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/clie...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/clie...
state/apiserver/client/client.go:150: // Split settings due to compatability
reasons.
On 2013/09/05 07:18:46, dimitern wrote:
> s/compatability/compatibility/

Done.

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/clie...
state/apiserver/client/client.go:155: // due to compatability.
On 2013/09/05 07:18:46, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/clie...
state/apiserver/client/client.go:254: // Split settings due to compatability
reasons.
On 2013/09/05 07:18:46, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/clie...
state/apiserver/client/client.go:260: // Validate and merge the unset settings,
again due to compatability.
On 2013/09/05 07:18:46, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/clie...
state/apiserver/client/client.go:261: err = esc.validateAndMerge(ch, changes)
On 2013/09/04 15:48:09, fwereade wrote:
> This block looks like it's repeated -- can we wrap the whole lot in a func?

Done.Yeah,by far more clear now.

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/clie...
state/apiserver/client/client.go:419: // emptySettingCompatability is a helper
type to keep the API behavior while the
On 2013/09/05 07:18:46, dimitern wrote:
> Please change s/compatability/compatibility/ across the whole file.

Done.

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/get_...
File state/apiserver/client/get_test.go (right):

https://codereview.appspot.com/13522043/diff/4001/state/apiserver/client/get_...
state/apiserver/client/get_test.go:93: config: map[string]string{
On 2013/09/04 15:48:09, fwereade wrote:
> Aren't we just using the wrong type here? If we set the *actual* config values
> we'll have a better test, I think.

Done.

[jameinel, 2013-09-10 08:33:24]

I'd like to see some clearer tests about what is happening. (A test that clearly
sets a value for a string field, whose default is not the empty string. Then
unsets the field and we see the final value is the default.)

Otherwise LGTM

https://codereview.appspot.com/13522043/diff/11001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/13522043/diff/11001/state/apiserver/client/cli...
state/apiserver/client/client.go:410: // unsetted the option. Now empty strings
are valid for string options
"unset" the option

Though maybe we want to say: "setting an option to an empty string caused it to
reset to the default value. We now allow empty strings as actual values, but we
want to preserve the API behavior."

https://codereview.appspot.com/13522043/diff/11001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (left):

https://codereview.appspot.com/13522043/diff/11001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:53: c.Assert(err, gc.IsNil)
It isn't very clear to me what this is doing to the "skill-level" field.

Also IIRC skill-level is an Int field, so I'm guessing it might have different
processing anyway.

Can you just add another test that just focuses on 1 API call that uses the
empty-string-means-default behavior?

Also, *I* would like to see some direct tests of the parseSettingsCompatible
function. It isn't required (because it isn't our habit to do so), but I think
having unit tests of what the function is expected to do (take things that have
empty string fields and turn them into nil fields) is useful.

[mue, 2013-09-11 08:17:58]

Please take a look.

https://codereview.appspot.com/13522043/diff/11001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/13522043/diff/11001/state/apiserver/client/cli...
state/apiserver/client/client.go:410: // unsetted the option. Now empty strings
are valid for string options
On 2013/09/10 08:33:24, jameinel wrote:
> "unset" the option
> 
> Though maybe we want to say: "setting an option to an empty string caused it
to
> reset to the default value. We now allow empty strings as actual values, but
we
> want to preserve the API behavior."

Done.

[axw1, 2013-09-11 08:37:42]

LGTM, just a couple of typos and stale comments.

https://codereview.appspot.com/13522043/diff/16001/charm/config.go
File charm/config.go (left):

https://codereview.appspot.com/13522043/diff/16001/charm/config.go#oldcode45
charm/config.go:45: if checker := optionTypeCheckers[option.Type]; checker !=
nil {
Comment above here (not seen in diff) needs to change.

https://codereview.appspot.com/13522043/diff/16001/charm/config.go#oldcode65
charm/config.go:65: // string values are returned as nil.
Comment needs to change.

https://codereview.appspot.com/13522043/diff/16001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/13522043/diff/16001/state/apiserver/client/cli...
state/apiserver/client/client.go:248: // Parse config in a compatile way (see
function comment).
typo - s/compatile/compatible/

https://codereview.appspot.com/13522043/diff/16001/state/apiserver/client/cli...
state/apiserver/client/client.go:408: // campatible with the behavior before
this CL based on the issue
s/campatible/compatible/

https://codereview.appspot.com/13522043/diff/16001/state/apiserver/client/exp...
File state/apiserver/client/export_test.go (right):

https://codereview.appspot.com/13522043/diff/16001/state/apiserver/client/exp...
state/apiserver/client/export_test.go:6: import ()
why is this here?

[mue, 2013-09-11 16:29:09]

https://codereview.appspot.com/13522043/diff/16001/charm/config.go
File charm/config.go (left):

https://codereview.appspot.com/13522043/diff/16001/charm/config.go#oldcode45
charm/config.go:45: if checker := optionTypeCheckers[option.Type]; checker !=
nil {
On 2013/09/11 08:37:42, axw1 wrote:
> Comment above here (not seen in diff) needs to change.

Done.

https://codereview.appspot.com/13522043/diff/16001/charm/config.go#oldcode65
charm/config.go:65: // string values are returned as nil.
On 2013/09/11 08:37:42, axw1 wrote:
> Comment needs to change.

Done.

https://codereview.appspot.com/13522043/diff/16001/state/apiserver/client/cli...
File state/apiserver/client/client.go (right):

https://codereview.appspot.com/13522043/diff/16001/state/apiserver/client/cli...
state/apiserver/client/client.go:248: // Parse config in a compatile way (see
function comment).
On 2013/09/11 08:37:42, axw1 wrote:
> typo - s/compatile/compatible/

Done.

https://codereview.appspot.com/13522043/diff/16001/state/apiserver/client/cli...
state/apiserver/client/client.go:408: // campatible with the behavior before
this CL based on the issue
On 2013/09/11 08:37:42, axw1 wrote:
> s/campatible/compatible/

Done.

https://codereview.appspot.com/13522043/diff/16001/state/apiserver/client/exp...
File state/apiserver/client/export_test.go (right):

https://codereview.appspot.com/13522043/diff/16001/state/apiserver/client/exp...
state/apiserver/client/export_test.go:6: import ()
On 2013/09/11 08:37:42, axw1 wrote:
> why is this here?

Done.