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
Merged

[jjo] diff: handle empty config values #180

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f828f2f
[jjo] diff: handle empty config values #179
jjo Dec 15, 2017
d94b343
add mmikulicic comment suggestions
jjo Dec 15, 2017
117999e
diff_test.go: test also nested nil-s
jjo Dec 19, 2017
486910c
use gus' isEmptyValue() implementation
jjo Dec 19, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions pkg/kubecfg/diff.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"fmt"
"io"
"os"
"reflect"
"sort"

isatty "github.com/mattn/go-isatty"
Expand Down Expand Up @@ -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.

// standard library.
func isEmptyValue(v reflect.Value) bool {
switch v.Kind() {
Copy link
Contributor

@anguslees anguslees Dec 19, 2017 •
edited
Loading

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

case reflect.Array, reflect.Map, reflect.Slice, reflect.String:
return v.Len() == 0
case reflect.Bool:
return !v.Bool()
case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64:
return v.Int() == 0
case reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Uintptr:
return v.Uint() == 0
case reflect.Float32, reflect.Float64:
return v.Float() == 0
case reflect.Interface, reflect.Ptr:
return v.IsNil()
}
return false
}

func removeFields(config, live interface{}) interface{} {
switch c := config.(type) {
case map[string]interface{}:
Expand All @@ -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.

result[k] = v1
}
continue
}
result[k] = removeFields(v1, v2)
Expand Down
15 changes: 15 additions & 0 deletions pkg/kubecfg/diff_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,21 @@ func TestRemoveFields(t *testing.T) {
live: "b",
expected: "b",
},
// Check we handle empty configs by copying them as if were live
// (API won't return them)
{
config: map[string]interface{}{
"args": map[string]interface{}{},
"volumes": []string{},
"stdin": false,
},
live: map[string]interface{}{},
expected: map[string]interface{}{
"args": map[string]interface{}{},
"volumes": []string{},
"stdin": false,
},
},

// Check we can handle combinations.
{
Expand Down