Sort objects prior to diff

Additional fixes:

  * sort order is constant for json and yaml output of `ks show`

Fixes #801
Signed-off-by: Oren Shomron <shomron@gmail.com>

pkg/cluster/show.go

@@ -19,11 +19,10 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
-	"sort"
 
+	"github.com/ghodss/yaml"
 	"github.com/ksonnet/ksonnet/pkg/app"
 	"github.com/pkg/errors"
-	yaml "gopkg.in/yaml.v2"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 )
 
@@ -68,11 +67,15 @@ func (s *Show) Show() error {
 		return errors.Wrap(err, "find objects")
 	}
 
+	sorted := make([]*unstructured.Unstructured, len(apiObjects))
+	copy(sorted, apiObjects)
+	UnstructuredSlice(sorted).Sort()
+
 	switch s.Format {
 	case "yaml":
-		return s.showYAML(apiObjects)
+		return s.showYAML(sorted)
 	case "json":
-		return s.showJSON(apiObjects)
+		return s.showJSON(sorted)
 	default:
 		return fmt.Errorf("Unknown --format: %s", s.Format)
 	}
@@ -104,29 +107,9 @@ func (s *Show) showJSON(apiObjects []*unstructured.Unstructured) error {
 
 // ShowYAML shows YAML objects.
 func ShowYAML(out io.Writer, apiObjects []*unstructured.Unstructured) error {
-	objects := make([]*unstructured.Unstructured, len(apiObjects))
-	for i := range apiObjects {
-		obj := apiObjects[i]
-		objects[i] = obj.DeepCopy()
-	}
-
-	sortByKind(objects)
-
-	for i := range objects {
-		obj := objects[i]
+	for _, obj := range apiObjects {
 		fmt.Fprintln(out, "---")
-		// Go via json because we need
-		// to trigger the custom scheme
-		// encoding.
-		buf, err := json.Marshal(obj)
-		if err != nil {
-			return err
-		}
-		o := map[string]interface{}{}
-		if err = json.Unmarshal(buf, &o); err != nil {
-			return err
-		}
-		buf, err = yaml.Marshal(o)
+		buf, err := yaml.Marshal(obj)
 		if err != nil {
 			return err
 		}
@@ -138,26 +121,3 @@ func ShowYAML(out io.Writer, apiObjects []*unstructured.Unstructured) error {
 
 	return nil
 }
-
-// sortByKind sorts objects by their kind/group/version/name
-func sortByKind(apiObjects []*unstructured.Unstructured) {
-	sort.SliceStable(apiObjects, func(i, j int) bool {
-		o1 := apiObjects[i]
-		o2 := apiObjects[j]
-
-		if o1.GroupVersionKind().Kind < o2.GroupVersionKind().Kind {
-			return true
-		}
-
-		if o1.GroupVersionKind().Group < o2.GroupVersionKind().Group {
-			return true
-		}
-
-		if o1.GroupVersionKind().Version < o2.GroupVersionKind().Version {
-			return true
-		}
-
-		return o1.GetName() < o2.GetName()
-	})
-
-}

pkg/cluster/show_test.go

@@ -106,107 +106,3 @@ func TestShow(t *testing.T) {
 		})
 	}
 }
-
-func Test_sortByKind(t *testing.T) {
-	objects := []*unstructured.Unstructured{
-		{
-			Object: map[string]interface{}{
-				"apiVersion": "apps/v1beta2",
-				"kind":       "Deployment",
-				"metadata": map[string]interface{}{
-					"name": "d3",
-				},
-			},
-		},
-		{
-			Object: map[string]interface{}{
-				"apiVersion": "v1",
-				"kind":       "Service",
-				"metadata": map[string]interface{}{
-					"name": "s2",
-				},
-			},
-		},
-		{
-			Object: map[string]interface{}{
-				"apiVersion": "apps/v1",
-				"kind":       "Deployment",
-				"metadata": map[string]interface{}{
-					"name": "d1",
-				},
-			},
-		},
-		{
-			Object: map[string]interface{}{
-				"apiVersion": "extensions/v1beta1",
-				"kind":       "Deployment",
-				"metadata": map[string]interface{}{
-					"name": "d2",
-				},
-			},
-		},
-		{
-			Object: map[string]interface{}{
-				"apiVersion": "v1",
-				"kind":       "Service",
-				"metadata": map[string]interface{}{
-					"name": "s1",
-				},
-			},
-		},
-	}
-
-	for i := 0; i < 10; i++ {
-		sortByKind(objects)
-	}
-
-	expected := []*unstructured.Unstructured{
-		{
-			Object: map[string]interface{}{
-				"apiVersion": "apps/v1",
-				"kind":       "Deployment",
-				"metadata": map[string]interface{}{
-					"name": "d1",
-				},
-			},
-		},
-		{
-			Object: map[string]interface{}{
-				"apiVersion": "apps/v1beta2",
-				"kind":       "Deployment",
-				"metadata": map[string]interface{}{
-					"name": "d3",
-				},
-			},
-		},
-		{
-			Object: map[string]interface{}{
-				"apiVersion": "extensions/v1beta1",
-				"kind":       "Deployment",
-				"metadata": map[string]interface{}{
-					"name": "d2",
-				},
-			},
-		},
-		{
-			Object: map[string]interface{}{
-				"apiVersion": "v1",
-				"kind":       "Service",
-				"metadata": map[string]interface{}{
-					"name": "s1",
-				},
-			},
-		},
-		{
-			Object: map[string]interface{}{
-				"apiVersion": "v1",
-				"kind":       "Service",
-				"metadata": map[string]interface{}{
-					"name": "s2",
-				},
-			},
-		},
-	}
-
-	require.Equal(t, expected, objects)
-}

pkg/cluster/sort.go

+// Copyright 2018 The ksonnet authors
+//
+//
+//    Licensed under the Apache License, Version 2.0 (the "License");
+//    you may not use this file except in compliance with the License.
+//    You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+//    Unless required by applicable law or agreed to in writing, software
+//    distributed under the License is distributed on an "AS IS" BASIS,
+//    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+//    See the License for the specific language governing permissions and
+//    limitations under the License.
+
+package cluster
+
+import (
+	"sort"
+
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+)
+
+// UnstructuredSlice is a sortable slice of k8s unstructured.Unstructured objects
+type UnstructuredSlice []*unstructured.Unstructured
+
+// Sort sorts an UnstructuredSlice
+func (u UnstructuredSlice) Sort() {
+	sort.Stable(u)
+}
+func (u UnstructuredSlice) Len() int {
+	return len(u)
+}
+func (u UnstructuredSlice) Swap(i, j int) {
+	u[i], u[j] = u[j], u[i]
+}
+func (u UnstructuredSlice) Less(i, j int) bool {
+	// Ordered sort key extractors
+	keyFuncs := []func(*unstructured.Unstructured) string{
+		func(o *unstructured.Unstructured) string {
+			return o.GetNamespace()
+		},
+		func(o *unstructured.Unstructured) string {
+			return o.GroupVersionKind().String()
+		},
+		func(o *unstructured.Unstructured) string {
+			return o.GetName()
+		},
+		func(o *unstructured.Unstructured) string {
+			return o.GetGenerateName()
+		},
+		func(o *unstructured.Unstructured) string {
+			return string(o.GetUID())
+		},
+	}
+
+	a := u[i]
+	b := u[j]
+
+	switch {
+	case a == nil:
+		return true
+	case b == nil:
+		return false
+	}
+
+	for _, f := range keyFuncs {
+		vA, vB := f(a), f(b)
+		switch {
+		case vA < vB:
+			return true
+		case vA > vB:
+			return false
+		}
+	}
+
+	return false
+}

pkg/cluster/sort_test.go

+// Copyright 2018 The ksonnet authors
+//
+//
+//    Licensed under the Apache License, Version 2.0 (the "License");
+//    you may not use this file except in compliance with the License.
+//    You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+//    Unless required by applicable law or agreed to in writing, software
+//    distributed under the License is distributed on an "AS IS" BASIS,
+//    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+//    See the License for the specific language governing permissions and
+//    limitations under the License.
+
+package cluster
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+)
+
+func Test_UnstructuredSlice_Sort(t *testing.T) {
+	objects := []*unstructured.Unstructured{
+		{
+			Object: map[string]interface{}{
+				"apiVersion": "apps/v1beta2",
+				"kind":       "Deployment",
+				"metadata": map[string]interface{}{
+					"name": "d3",
+				},
+			},
+		},
+		{
+			Object: map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "Service",
+				"metadata": map[string]interface{}{
+					"name": "s2",
+				},
+			},
+		},
+		{
+			Object: map[string]interface{}{
+				"apiVersion": "apps/v1",
+				"kind":       "Deployment",
+				"metadata": map[string]interface{}{
+					"name": "d1",
+				},
+			},
+		},
+		{
+			Object: map[string]interface{}{
+				"apiVersion": "extensions/v1beta1",
+				"kind":       "Deployment",
+				"metadata": map[string]interface{}{
+					"name": "d2",
+				},
+			},
+		},
+		{
+			Object: map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "Service",
+				"metadata": map[string]interface{}{
+					"name": "s1",
+				},
+			},
+		},
+	}
+
+	expected := []*unstructured.Unstructured{
+		{
+			Object: map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "Service",
+				"metadata": map[string]interface{}{
+					"name": "s1",
+				},
+			},
+		},
+		{
+			Object: map[string]interface{}{
+				"apiVersion": "v1",
+				"kind":       "Service",
+				"metadata": map[string]interface{}{
+					"name": "s2",
+				},
+			},
+		},
+		{
+			Object: map[string]interface{}{
+				"apiVersion": "apps/v1",
+				"kind":       "Deployment",
+				"metadata": map[string]interface{}{
+					"name": "d1",
+				},
+			},
+		},
+		{
+			Object: map[string]interface{}{
+				"apiVersion": "apps/v1beta2",
+				"kind":       "Deployment",
+				"metadata": map[string]interface{}{
+					"name": "d3",
+				},
+			},
+		},
+		{
+			Object: map[string]interface{}{
+				"apiVersion": "extensions/v1beta1",
+				"kind":       "Deployment",
+				"metadata": map[string]interface{}{
+					"name": "d2",
+				},
+			},
+		},
+	}
+
+	for i := 0; i < 10; i++ {
+		UnstructuredSlice(objects).Sort()
+	}
+
+	require.Equal(t, expected, objects)
+}

pkg/diff/diff.go

@@ -22,6 +22,7 @@ import (
 	"github.com/ksonnet/ksonnet/pkg/app"
 	"github.com/ksonnet/ksonnet/pkg/client"
 	"github.com/ksonnet/ksonnet/pkg/cluster"
+	"github.com/ksonnet/ksonnet/pkg/pipeline"
 	"github.com/pkg/errors"
 	godiff "github.com/shazow/go-diff"
 	"github.com/sirupsen/logrus"
@@ -106,29 +107,36 @@ type yamlGenerator interface {
 }
 
 type yamlLocal struct {
-	app    app.App
-	showFn func(cluster.ShowConfig, ...cluster.ShowOpts) error
+	app              app.App
+	collectObjectsFn func(a app.App, envName string, componentNames []string) ([]*unstructured.Unstructured, error)
+	showFn           func(io.Writer, []*unstructured.Unstructured) error
 }
 
 func newYamlLocal(a app.App) *yamlLocal {
 	return &yamlLocal{
-		app:    a,
-		showFn: cluster.RunShow,
+		app:              a,
+		collectObjectsFn: localCollectObjects,
+		showFn:           cluster.ShowYAML,
 	}
 }
 
+func localCollectObjects(a app.App, envName string, componentNames []string) ([]*unstructured.Unstructured, error) {
+	p := pipeline.New(a, envName)
+	return p.Objects(componentNames)
+}
+
 func (yl *yamlLocal) Generate(location *Location, components []string) (io.ReadSeeker, error) {
 	var buf bytes.Buffer
 
-	showConfig := cluster.ShowConfig{
-		App:            yl.app,
-		EnvName:        location.EnvName(),
-		Format:         "yaml",
-		Out:            &buf,
-		ComponentNames: components,
+	objects, err := yl.collectObjectsFn(yl.app, location.EnvName(), components)
+	if err != nil {
+		return nil, err
+
 	}
 
-	if err := yl.showFn(showConfig); err != nil {
+	cluster.UnstructuredSlice(objects).Sort()
+
+	if err := yl.showFn(&buf, objects); err != nil {
 		return nil, err
 	}
 
@@ -164,6 +172,8 @@ func (yr *yamlRemote) Generate(location *Location, components []string) (io.Read
 		return nil, err
 	}
 
+	cluster.UnstructuredSlice(objects).Sort()
+
 	if err := yr.showFn(&buf, objects); err != nil {
 		return nil, err
 	}

pkg/diff/diff_test.go

@@ -22,10 +22,10 @@ import (
 	"io/ioutil"
 	"testing"
 
+	"github.com/ghodss/yaml"
 	"github.com/ksonnet/ksonnet/pkg/app"
 	"github.com/ksonnet/ksonnet/pkg/app/mocks"
 	"github.com/ksonnet/ksonnet/pkg/client"
-	"github.com/ksonnet/ksonnet/pkg/cluster"
 	"github.com/ksonnet/ksonnet/pkg/util/test"
 	"github.com/pkg/errors"
 	"github.com/spf13/afero"
@@ -77,24 +77,41 @@ func TestDiffer(t *testing.T) {
 
 func Test_yamlLocal(t *testing.T) {
 	cases := []struct {
-		name   string
-		showFn func(c cluster.ShowConfig, opts ...cluster.ShowOpts) error
-		isErr  bool
+		name             string
+		collectObjectsFn func(a app.App, envName string, componentNames []string) ([]*unstructured.Unstructured, error)
+		showFn           func(io.Writer, []*unstructured.Unstructured) error
+		expected         string
+		isErr            bool
 	}{
 		{
 			name: "in general",
-			showFn: func(c cluster.ShowConfig, opts ...cluster.ShowOpts) error {
-				fmt.Fprint(c.Out, "output")
+			collectObjectsFn: func(a app.App, envName string, componentNames []string) ([]*unstructured.Unstructured, error) {
+				return nil, nil
+			},
+			showFn: func(w io.Writer, objects []*unstructured.Unstructured) error {
+				fmt.Fprint(w, "output")
 				return nil
 			},
+			expected: "output",
 		},
 		{
 			name: "show failed",
-			showFn: func(c cluster.ShowConfig, opts ...cluster.ShowOpts) error {
+			collectObjectsFn: func(a app.App, envName string, componentNames []string) ([]*unstructured.Unstructured, error) {
+				return nil, nil
+			},
+			showFn: func(w io.Writer, objects []*unstructured.Unstructured) error {
 				return errors.New("fail")
 			},
 			isErr: true,
 		},
+		{
+			name: "sorted",
+			collectObjectsFn: func(a app.App, envName string, componentNames []string) ([]*unstructured.Unstructured, error) {
+				return genObjects(), nil
+			},
+			showFn:   showYAML,
+			expected: sortedYAML,
+		},
 	}
 
 	for _, tc := range cases {
@@ -104,6 +121,7 @@ func Test_yamlLocal(t *testing.T) {
 
 				yl := newYamlLocal(appMock)
 
+				yl.collectObjectsFn = tc.collectObjectsFn
 				yl.showFn = tc.showFn
 
 				rs, err := yl.Generate(location, []string{})
@@ -116,7 +134,7 @@ func Test_yamlLocal(t *testing.T) {
 				b, err := ioutil.ReadAll(rs)
 				require.NoError(t, err)
 
-				require.Equal(t, "output", string(b))
+				require.Equal(t, tc.expected, string(b))
 			})
 		})
 	}
@@ -138,6 +156,7 @@ func Test_yamlRemote(t *testing.T) {
 		appSetup  func(a *mocks.App)
 		collectFn func(namespace string, config clientcmd.ClientConfig, components []string) ([]*unstructured.Unstructured, error)
 		showFn    func(w io.Writer, objects []*unstructured.Unstructured) error
+		expected  string
 		isErr     bool
 	}{
 		{
@@ -150,8 +169,8 @@ func Test_yamlRemote(t *testing.T) {
 				fmt.Fprintf(w, "output")
 				return nil
 			},
+			expected: "output",
 		},
-
 		{
 			name: "invalid environment",
 			appSetup: func(a *mocks.App) {
@@ -159,7 +178,6 @@ func Test_yamlRemote(t *testing.T) {
 			},
 			isErr: true,
 		},
-
 		{
 			name:     "collect objects failed",
 			appSetup: validAppSetup,
@@ -168,7 +186,6 @@ func Test_yamlRemote(t *testing.T) {
 			},
 			isErr: true,
 		},
-
 		{
 			name:     "show failed",
 			appSetup: validAppSetup,
@@ -180,6 +197,15 @@ func Test_yamlRemote(t *testing.T) {
 			},
 			isErr: true,
 		},
+		{
+			name:     "sorted",
+			appSetup: validAppSetup,
+			collectFn: func(namespace string, config clientcmd.ClientConfig, components []string) ([]*unstructured.Unstructured, error) {
+				return genObjects(), nil
+			},
+			showFn:   showYAML,
+			expected: sortedYAML,
+		},
 	}
 
 	for _, tc := range cases {
@@ -205,8 +231,137 @@ func Test_yamlRemote(t *testing.T) {
 				b, err := ioutil.ReadAll(rs)
 				require.NoError(t, err)
 
-				require.Equal(t, "output", string(b))
+				require.Equal(t, tc.expected, string(b))
 			})
 		})
 	}
 }
+
+func showYAML(out io.Writer, objects []*unstructured.Unstructured) error {
+	for _, obj := range objects {
+		fmt.Fprintln(out, "---")
+		buf, err := yaml.Marshal(obj)
+		if err != nil {
+			return err
+		}
+		_, err = out.Write(buf)
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func genObjects() []*unstructured.Unstructured {
+	return []*unstructured.Unstructured{
+		&unstructured.Unstructured{
+			Object: map[string]interface{}{
+				"apiVersion": "apps/v1",
+				"kind":       "Deployment",
+				"metadata": map[string]interface{}{
+					"name":      "deploymentZ",
+					"namespace": "default",
+					"annotations": map[string]interface{}{