Merge pull request #68525 from apelisse/kubectl-diff-simplify-interface

hack/testdata/pod-changed.yaml

+apiVersion: v1
+kind: Pod
+metadata:
+  name: test-pod
+  labels:
+    name: test-pod-label
+spec:
+  containers:
+  - name: kubernetes-pause
+    image: k8s.gcr.io/pause:3.0

pkg/kubectl/cmd/diff.go

@@ -44,11 +44,8 @@ import (
 
 var (
 	diffLong = templates.LongDesc(i18n.T(`
-		Diff configurations specified by filename or stdin between their local,
-		last-applied, live and/or "merged" versions.
-
-		LOCAL and LIVE versions are diffed by default. Other available keywords
-		are MERGED and LAST.
+		Diff configurations specified by filename or stdin between the current online
+		configuration, and the configuration as it would be if applied.
 
 		Output is always YAML.
 
@@ -56,52 +53,22 @@ var (
 		diff command. By default, the "diff" command available in your path will be
 		run with "-u" (unicode) and "-N" (treat new files as empty) options.`))
 	diffExample = templates.Examples(i18n.T(`
-		# Diff resources included in pod.json. By default, it will diff LOCAL and LIVE versions
+		# Diff resources included in pod.json.
 		kubectl alpha diff -f pod.json
 
-		# When one version is specified, diff that version against LIVE
-		cat service.yaml | kubectl alpha diff -f - MERGED
-
-		# Or specify both versions
-		kubectl alpha diff -f pod.json -f service.yaml LAST LOCAL`))
+		# Diff file read from stdin
+		cat service.yaml | kubectl alpha diff -f -`))
 )
 
 type DiffOptions struct {
 	FilenameOptions resource.FilenameOptions
 }
 
-func isValidArgument(arg string) error {
-	switch arg {
-	case "LOCAL", "LIVE", "LAST", "MERGED":
-		return nil
-	default:
-		return fmt.Errorf(`Invalid parameter %q, must be either "LOCAL", "LIVE", "LAST" or "MERGED"`, arg)
-	}
-
-}
-
-func parseDiffArguments(args []string) (string, string, error) {
-	if len(args) > 2 {
-		return "", "", fmt.Errorf("Invalid number of arguments: expected at most 2.")
-	}
-	// Default values
-	from := "LOCAL"
-	to := "LIVE"
-	if len(args) > 0 {
-		from = args[0]
-	}
-	if len(args) > 1 {
-		to = args[1]
-	}
-
-	if err := isValidArgument(to); err != nil {
-		return "", "", err
+func checkDiffArgs(cmd *cobra.Command, args []string) error {
+	if len(args) != 0 {
+		return cmdutil.UsageErrorf(cmd, "Unexpected args: %v", args)
 	}
-	if err := isValidArgument(from); err != nil {
-		return "", "", err
-	}
-
-	return from, to, nil
+	return nil
 }
 
 func NewCmdDiff(f cmdutil.Factory, streams genericclioptions.IOStreams) *cobra.Command {
@@ -113,13 +80,12 @@ func NewCmdDiff(f cmdutil.Factory, streams genericclioptions.IOStreams) *cobra.C
 	cmd := &cobra.Command{
 		Use: "diff -f FILENAME",
 		DisableFlagsInUseLine: true,
-		Short:   i18n.T("Diff different versions of configurations"),
+		Short:   i18n.T("Diff live version against would-be applied version"),
 		Long:    diffLong,
 		Example: diffExample,
 		Run: func(cmd *cobra.Command, args []string) {
-			from, to, err := parseDiffArguments(args)
-			cmdutil.CheckErr(err)
-			cmdutil.CheckErr(RunDiff(f, &diff, &options, from, to))
+			cmdutil.CheckErr(checkDiffArgs(cmd, args))
+			cmdutil.CheckErr(RunDiff(f, &diff, &options))
 		},
 	}
 
@@ -201,10 +167,6 @@ func (v *DiffVersion) getObject(obj Object) (map[string]interface{}, error) {
 		return obj.Live()
 	case "MERGED":
 		return obj.Merged()
-	case "LOCAL":
-		return obj.Local()
-	case "LAST":
-		return obj.Last()
 	}
 	return nil, fmt.Errorf("Unknown version: %v", v.Name)
 }
@@ -254,9 +216,7 @@ func (d *Directory) Delete() error {
 // Object is an interface that let's you retrieve multiple version of
 // it.
 type Object interface {
-	Local() (map[string]interface{}, error)
 	Live() (map[string]interface{}, error)
-	Last() (map[string]interface{}, error)
 	Merged() (map[string]interface{}, error)
 
 	Name() string
@@ -282,14 +242,6 @@ func (obj InfoObject) toMap(data []byte) (map[string]interface{}, error) {
 	return m, err
 }
 
-func (obj InfoObject) Local() (map[string]interface{}, error) {
-	data, err := runtime.Encode(obj.Encoder, obj.Info.Object)
-	if err != nil {
-		return nil, err
-	}
-	return obj.toMap(data)
-}
-
 func (obj InfoObject) Live() (map[string]interface{}, error) {
 	if obj.Remote == nil {
 		return nil, nil // Object doesn't exist on cluster.
@@ -298,10 +250,11 @@ func (obj InfoObject) Live() (map[string]interface{}, error) {
 }
 
 func (obj InfoObject) Merged() (map[string]interface{}, error) {
-	local, err := obj.Local()
+	data, err := runtime.Encode(obj.Encoder, obj.Info.Object)
 	if err != nil {
 		return nil, err
 	}
+	local, err := obj.toMap(data)
 
 	live, err := obj.Live()
 	if err != nil {
@@ -436,14 +389,14 @@ func (d *Downloader) Download(info *resource.Info) (*unstructured.Unstructured,
 // RunDiff uses the factory to parse file arguments, find the version to
 // diff, and find each Info object for each files, and runs against the
 // differ.
-func RunDiff(f cmdutil.Factory, diff *DiffProgram, options *DiffOptions, from, to string) error {
+func RunDiff(f cmdutil.Factory, diff *DiffProgram, options *DiffOptions) error {
 	openapi, err := f.OpenAPISchema()
 	if err != nil {
 		return err
 	}
 	parser := &parse.Factory{Resources: openapi}
 
-	differ, err := NewDiffer(from, to)
+	differ, err := NewDiffer("LIVE", "MERGED")
 	if err != nil {
 		return err
 	}

pkg/kubectl/cmd/diff_test.go

@@ -31,10 +31,8 @@ import (
 
 type FakeObject struct {
 	name   string
-	local  map[string]interface{}
 	merged map[string]interface{}
 	live   map[string]interface{}
-	last   map[string]interface{}
 }
 
 var _ Object = &FakeObject{}
@@ -43,10 +41,6 @@ func (f *FakeObject) Name() string {
 	return f.name
 }
 
-func (f *FakeObject) Local() (map[string]interface{}, error) {
-	return f.local, nil
-}
-
 func (f *FakeObject) Merged() (map[string]interface{}, error) {
 	return f.merged, nil
 }
@@ -55,87 +49,6 @@ func (f *FakeObject) Live() (map[string]interface{}, error) {
 	return f.live, nil
 }
 
-func (f *FakeObject) Last() (map[string]interface{}, error) {
-	return f.last, nil
-}
-
-func TestArguments(t *testing.T) {
-	tests := []struct {
-		// Input
-		args []string
-
-		// Outputs
-		from string
-		to   string
-		err  string
-	}{
-		// Defaults
-		{
-			args: []string{},
-			from: "LOCAL",
-			to:   "LIVE",
-			err:  "",
-		},
-		// One valid argument
-		{
-			args: []string{"MERGED"},
-			from: "MERGED",
-			to:   "LIVE",
-			err:  "",
-		},
-		// One invalid argument
-		{
-			args: []string{"WRONG"},
-			from: "",
-			to:   "",
-			err:  `Invalid parameter "WRONG", must be either "LOCAL", "LIVE", "LAST" or "MERGED"`,
-		},
-		// Two valid arguments
-		{
-			args: []string{"MERGED", "LAST"},
-			from: "MERGED",
-			to:   "LAST",
-			err:  "",
-		},
-		// Two same arguments is fine
-		{
-			args: []string{"MERGED", "MERGED"},
-			from: "MERGED",
-			to:   "MERGED",
-			err:  "",
-		},
-		// Second argument is invalid
-		{
-			args: []string{"MERGED", "WRONG"},
-			from: "",
-			to:   "",
-			err:  `Invalid parameter "WRONG", must be either "LOCAL", "LIVE", "LAST" or "MERGED"`,
-		},
-		// Three arguments
-		{
-			args: []string{"MERGED", "LIVE", "LAST"},
-			from: "",
-			to:   "",
-			err:  `Invalid number of arguments: expected at most 2.`,
-		},
-	}
-
-	for _, test := range tests {
-		from, to, e := parseDiffArguments(test.args)
-		err := ""
-		if e != nil {
-			err = e.Error()
-		}
-		if from != test.from || to != test.to || err != test.err {
-			t.Errorf("parseDiffArguments(%v) = (%v, %v, %v), expected (%v, %v, %v)",
-				test.args,
-				from, to, err,
-				test.from, test.to, test.err,
-			)
-		}
-	}
-}
-
 func TestDiffProgram(t *testing.T) {
 	os.Setenv("KUBERNETES_EXTERNAL_DIFF", "echo")
 	streams, _, stdout, _ := genericclioptions.NewTestIOStreams()
@@ -175,7 +88,7 @@ string: string
 }
 
 func TestDiffVersion(t *testing.T) {
-	diff, err := NewDiffVersion("LOCAL")
+	diff, err := NewDiffVersion("MERGED")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -183,8 +96,6 @@ func TestDiffVersion(t *testing.T) {
 
 	obj := FakeObject{
 		name:   "bla",
-		local:  map[string]interface{}{"local": true},
-		last:   map[string]interface{}{"last": true},
 		live:   map[string]interface{}{"live": true},
 		merged: map[string]interface{}{"merged": true},
 	}
@@ -196,7 +107,7 @@ func TestDiffVersion(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	econtent := "local: true\n"
+	econtent := "merged: true\n"
 	if string(fcontent) != econtent {
 		t.Fatalf("File has %q, expected %q", string(fcontent), econtent)
 	}
@@ -248,7 +159,7 @@ func TestDirectory(t *testing.T) {
 }
 
 func TestDiffer(t *testing.T) {
-	diff, err := NewDiffer("LOCAL", "LIVE")
+	diff, err := NewDiffer("LIVE", "MERGED")
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -256,8 +167,6 @@ func TestDiffer(t *testing.T) {
 
 	obj := FakeObject{
 		name:   "bla",
-		local:  map[string]interface{}{"local": true},
-		last:   map[string]interface{}{"last": true},
 		live:   map[string]interface{}{"live": true},
 		merged: map[string]interface{}{"merged": true},
 	}
@@ -269,7 +178,7 @@ func TestDiffer(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	econtent := "local: true\n"
+	econtent := "live: true\n"
 	if string(fcontent) != econtent {
 		t.Fatalf("File has %q, expected %q", string(fcontent), econtent)
 	}
@@ -278,7 +187,7 @@ func TestDiffer(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	econtent = "live: true\n"
+	econtent = "merged: true\n"
 	if string(fcontent) != econtent {
 		t.Fatalf("File has %q, expected %q", string(fcontent), econtent)
 	}

test/cmd/diff.sh

@@ -27,27 +27,13 @@ run_kubectl_diff_tests() {
     kube::log::status "Testing kubectl alpha diff"
 
     # Test that it works when the live object doesn't exist
-    output_message=$(kubectl alpha diff LOCAL LIVE -f hack/testdata/pod.yaml)
+    output_message=$(kubectl alpha diff -f hack/testdata/pod.yaml)
     kube::test::if_has_string "${output_message}" 'test-pod'
 
     kubectl apply -f hack/testdata/pod.yaml
 
-    # Ensure that selfLink has been added, and shown in the diff
-    output_message=$(kubectl alpha diff -f hack/testdata/pod.yaml)
-    kube::test::if_has_string "${output_message}" 'selfLink'
-    output_message=$(kubectl alpha diff LOCAL LIVE -f hack/testdata/pod.yaml)
-    kube::test::if_has_string "${output_message}" 'selfLink'
-    output_message=$(kubectl alpha diff LOCAL MERGED -f hack/testdata/pod.yaml)
-    kube::test::if_has_string "${output_message}" 'selfLink'
-
-    output_message=$(kubectl alpha diff MERGED MERGED -f hack/testdata/pod.yaml)
-    kube::test::if_empty_string "${output_message}"
-    output_message=$(kubectl alpha diff LIVE LIVE -f hack/testdata/pod.yaml)
-    kube::test::if_empty_string "${output_message}"
-    output_message=$(kubectl alpha diff LAST LAST -f hack/testdata/pod.yaml)
-    kube::test::if_empty_string "${output_message}"
-    output_message=$(kubectl alpha diff LOCAL LOCAL -f hack/testdata/pod.yaml)
-    kube::test::if_empty_string "${output_message}"
+    output_message=$(kubectl alpha diff -f hack/testdata/pod-changed.yaml)
+    kube::test::if_has_string "${output_message}" 'k8s.gcr.io/pause:3.0'
 
     kubectl delete -f  hack/testdata/pod.yaml