github
/
k3s
mirror of https://github.com/k3s-io/k3s
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

kubectl-diff: Simplify interface 

The current interface is kind of clunky and not super easy to use, since
you have to specify parameters to specify which versions to diff. Also
the default isn't the most useful setting.

Change the interface by removing all the parameters and force only one
useful use-case, that is: diffing what's currently live against
what would be live if applied.
pull/58/head
Antoine Pelisse 2018-09-11 14:32:01 -07:00
parent 7bfd0d358c
commit 0db6249740
4 changed files with 34 additions and 176 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
hack/testdata/pod-changed.yaml vendored Normal file
View File

@ -0,0 +1,10 @@
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

79
pkg/kubectl/cmd/diff.go
View File

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

101
pkg/kubectl/cmd/diff_test.go
View File

@ -31,10 +31,8 @@ import (
type FakeObject struct { type FakeObject struct {
name string name string
local map[string]interface{}
merged map[string]interface{} merged map[string]interface{}
live map[string]interface{} live map[string]interface{}
last map[string]interface{}
} }
var _ Object = &FakeObject{} var _ Object = &FakeObject{}
@ -43,10 +41,6 @@ func (f *FakeObject) Name() string {
return f.name return f.name
} }
func (f *FakeObject) Local() (map[string]interface{}, error) {
return f.local, nil
}
func (f *FakeObject) Merged() (map[string]interface{}, error) { func (f *FakeObject) Merged() (map[string]interface{}, error) {
return f.merged, nil return f.merged, nil
} }
@ -55,87 +49,6 @@ func (f *FakeObject) Live() (map[string]interface{}, error) {
return f.live, nil 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) { func TestDiffProgram(t *testing.T) {
os.Setenv("KUBERNETES_EXTERNAL_DIFF", "echo") os.Setenv("KUBERNETES_EXTERNAL_DIFF", "echo")
streams, _, stdout, _ := genericclioptions.NewTestIOStreams() streams, _, stdout, _ := genericclioptions.NewTestIOStreams()
@ -175,7 +88,7 @@ string: string
} }
func TestDiffVersion(t *testing.T) { func TestDiffVersion(t *testing.T) {
diff, err := NewDiffVersion("LOCAL") diff, err := NewDiffVersion("MERGED")
if err != nil { if err != nil {
t.Fatal(err) t.Fatal(err)
} }
@ -183,8 +96,6 @@ func TestDiffVersion(t *testing.T) {
obj := FakeObject{ obj := FakeObject{
name: "bla", name: "bla",
local: map[string]interface{}{"local": true},
last: map[string]interface{}{"last": true},
live: map[string]interface{}{"live": true}, live: map[string]interface{}{"live": true},
merged: map[string]interface{}{"merged": true}, merged: map[string]interface{}{"merged": true},
} }
@ -196,7 +107,7 @@ func TestDiffVersion(t *testing.T) {
if err != nil { if err != nil {
t.Fatal(err) t.Fatal(err)
} }
econtent := "local: true\n" econtent := "merged: true\n"
if string(fcontent) != econtent { if string(fcontent) != econtent {
t.Fatalf("File has %q, expected %q", 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) { func TestDiffer(t *testing.T) {
diff, err := NewDiffer("LOCAL", "LIVE") diff, err := NewDiffer("LIVE", "MERGED")
if err != nil { if err != nil {
t.Fatal(err) t.Fatal(err)
} }
@ -256,8 +167,6 @@ func TestDiffer(t *testing.T) {
obj := FakeObject{ obj := FakeObject{
name: "bla", name: "bla",
local: map[string]interface{}{"local": true},
last: map[string]interface{}{"last": true},
live: map[string]interface{}{"live": true}, live: map[string]interface{}{"live": true},
merged: map[string]interface{}{"merged": true}, merged: map[string]interface{}{"merged": true},
} }
@ -269,7 +178,7 @@ func TestDiffer(t *testing.T) {
if err != nil { if err != nil {
t.Fatal(err) t.Fatal(err)
} }
econtent := "local: true\n" econtent := "live: true\n"
if string(fcontent) != econtent { if string(fcontent) != econtent {
t.Fatalf("File has %q, expected %q", 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 { if err != nil {
t.Fatal(err) t.Fatal(err)
} }
econtent = "live: true\n" econtent = "merged: true\n"
if string(fcontent) != econtent { if string(fcontent) != econtent {
t.Fatalf("File has %q, expected %q", string(fcontent), econtent) t.Fatalf("File has %q, expected %q", string(fcontent), econtent)
} }

20
test/cmd/diff.sh
View File

@ -27,27 +27,13 @@ run_kubectl_diff_tests() {
kube::log::status "Testing kubectl alpha diff" kube::log::status "Testing kubectl alpha diff"
# Test that it works when the live object doesn't exist # 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' kube::test::if_has_string "${output_message}" 'test-pod'
kubectl apply -f hack/testdata/pod.yaml 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-changed.yaml)
output_message=$(kubectl alpha diff -f hack/testdata/pod.yaml) kube::test::if_has_string "${output_message}" 'k8s.gcr.io/pause:3.0'
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}"
kubectl delete -f hack/testdata/pod.yaml kubectl delete -f hack/testdata/pod.yaml