-
Notifications
You must be signed in to change notification settings - Fork 62
[jjo] diff: handle empty config values #180
[jjo] diff: handle empty config values #180
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
very minor comments
pkg/kubecfg/diff.go
Outdated
@@ -102,6 +103,26 @@ func (c DiffCmd) Run(apiObjects []*unstructured.Unstructured, out io.Writer) err | |||
return nil | |||
} | |||
|
|||
// isEmptyValue is taken from the encoding/json package in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please mention golang/go#7501
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, done.
pkg/kubecfg/diff.go
Outdated
@@ -118,6 +139,9 @@ func removeMapFields(config, live map[string]interface{}) map[string]interface{} | |||
for k, v1 := range config { | |||
v2, ok := live[k] | |||
if !ok { | |||
if isEmptyValue(reflect.ValueOf(v1)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps a comment wouldn't hurt here (so one doesn't have to git blame through the full future chain of commits until one hits this PR with the relevant context)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, done.
pkg/kubecfg/diff.go
Outdated
// See also feature request for golang reflect pkg at | ||
// https://github.com/golang/go/issues/7501 | ||
func isEmptyValue(v reflect.Value) bool { | ||
switch v.Kind() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw, this only has to deal with the types that encoding/json
produces, so a simple typeswitch will be faster:
func isEmptyValue(i interface{}) bool {
switch v := i.(type) {
case []interface{}:
return len(v) == 0
case map[string]interface{}:
return len(v) == 0
case bool:
return !v
case float64:
return v == 0
case string:
return v == ""
case nil:
return true
default:
panic("Found unexpected type %T in json unmarshal (value=%v)", i, i)
}
}
... and then invoke with isEmptyValue(v1)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anguslees: nopes, that won't support nested nil
s - I've augmented
diff_test.go
a bit to exercise that also.
FWIW using above simpler implementation would expectedly fail with:
# github.com/ksonnet/kubecfg/pkg/kubecfg
pkg/kubecfg/diff.go:142:8: too many arguments to panic: panic("Found unexpected type %T in json unmarshal (value=%v)", i, i)
Makefile:35: recipe for target 'kubecfg' failed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anguslees nvm on above reply (early morning low caffeine disclaimer :P),
re-implemented it using your suggestions above (+couple nits).
Fixes #179.