Skip to content
This repository has been archived by the owner on Nov 17, 2021. It is now read-only.

[jjo] diff: handle empty config values #180

Merged
merged 4 commits into from
Dec 20, 2017

Conversation

jjo
Copy link
Contributor

@jjo jjo commented Dec 15, 2017

Fixes #179.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link
Contributor

@mkmik mkmik left a 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

@@ -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
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, done.

@@ -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)) {
Copy link
Contributor

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, done.

// 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() {
Copy link
Contributor

@anguslees anguslees Dec 19, 2017

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)

Copy link
Contributor Author

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 nils - 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

Copy link
Contributor Author

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).

@anguslees anguslees merged commit 0870136 into vmware-archive:master Dec 20, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants