Merge pull request #2309 from lavalamp/fix3

Refactors of kubectl object printers.

hack/e2e.go

@@ -31,15 +31,19 @@ import (
 )
 
 var (
-	isup    = flag.Bool("isup", false, "Check to see if the e2e cluster is up, then exit.")
+	isup             = flag.Bool("isup", false, "Check to see if the e2e cluster is up, then exit.")
-	build   = flag.Bool("build", false, "If true, build a new release. Otherwise, use whatever is there.")
+	build            = flag.Bool("build", false, "If true, build a new release. Otherwise, use whatever is there.")
-	up      = flag.Bool("up", false, "If true, start the the e2e cluster. If cluster is already up, recreate it.")
+	up               = flag.Bool("up", false, "If true, start the the e2e cluster. If cluster is already up, recreate it.")
-	push    = flag.Bool("push", false, "If true, push to e2e cluster. Has no effect if -up is true.")
+	push             = flag.Bool("push", false, "If true, push to e2e cluster. Has no effect if -up is true.")
-	down    = flag.Bool("down", false, "If true, tear down the cluster before exiting.")
+	down             = flag.Bool("down", false, "If true, tear down the cluster before exiting.")
-	test    = flag.Bool("test", false, "Run all tests in hack/e2e-suite.")
+	test             = flag.Bool("test", false, "Run all tests in hack/e2e-suite.")
-	tests   = flag.String("tests", "", "Run only tests in hack/e2e-suite matching this glob. Ignored if -test is set.")
+	tests            = flag.String("tests", "", "Run only tests in hack/e2e-suite matching this glob. Ignored if -test is set.")
-	root    = flag.String("root", absOrDie(filepath.Clean(filepath.Join(path.Base(os.Args[0]), ".."))), "Root directory of kubernetes repository.")
+	root             = flag.String("root", absOrDie(filepath.Clean(filepath.Join(path.Base(os.Args[0]), ".."))), "Root directory of kubernetes repository.")
-	verbose = flag.Bool("v", false, "If true, print all command output.")
+	verbose          = flag.Bool("v", false, "If true, print all command output.")
+	checkVersionSkew = flag.Bool("check_version_skew", true, ""+
+		"By default, verify that client and server have exact version match. "+
+		"You can explicitly set to false if you're, e.g., testing client changes "+
+		"for which the server version doesn't make a difference.")
 
 	cfgCmd = flag.String("cfg", "", "If nonempty, pass this as an argument, and call kubecfg. Implies -v.")
 	ctlCmd = flag.String("ctl", "", "If nonempty, pass this as an argument, and call kubectl. Implies -v. (-test, -cfg, -ctl are mutually exclusive)")
@@ -95,7 +99,7 @@ func main() {
 	case *cfgCmd != "":
 		failure = !runBash("'kubecfg "+*cfgCmd+"'", "$KUBECFG "+*cfgCmd)
 	case *ctlCmd != "":
-		failure = !runBash("'kubectl "+*ctlCmd+"'", "$KUBECFG "+*ctlCmd)
+		failure = !runBash("'kubectl "+*ctlCmd+"'", "$KUBECTL "+*ctlCmd)
 	case *tests != "":
 		failed, passed := Test()
 		log.Printf("Passed tests: %v", passed)
@@ -210,7 +214,17 @@ func finishRunning(stepName string, cmd *exec.Cmd) bool {
 	return true
 }
 
-var bashCommandPrefix = `
+// returns either "", or a list of args intended for appending with the
+// kubecfg or kubectl commands (begining with a space).
+func kubeClientArgs() string {
+	if *checkVersionSkew {
+		return " -expect_version_match"
+	}
+	return ""
+}
+
+func bashWrap(cmd string) string {
+	return `
 set -o errexit
 set -o nounset
 set -o pipefail
@@ -219,19 +233,14 @@ export KUBE_CONFIG_FILE="config-test.sh"
 
 # TODO(jbeda): This will break on usage if there is a space in
 # ${KUBE_ROOT}.  Covert to an array?  Or an exported function?
-export KUBECFG="` + *root + `/cluster/kubecfg.sh -expect_version_match"
+export KUBECFG="` + *root + `/cluster/kubecfg.sh` + kubeClientArgs() + `"
+export KUBECTL="` + *root + `/cluster/kubectl.sh` + kubeClientArgs() + `"
 
 source "` + *root + `/cluster/kube-env.sh"
 source "` + *root + `/cluster/${KUBERNETES_PROVIDER}/util.sh"
 
 prepare-e2e
 
+` + cmd + `
 `
-
-var bashCommandSuffix = `
-
-`
-
-func bashWrap(cmd string) string {
-	return bashCommandPrefix + cmd + bashCommandSuffix
 }

pkg/kubectl/cmd/get.go

@@ -61,22 +61,16 @@ Examples:
 			defaultPrinter, err := f.Printer(cmd, mapping, GetFlagBool(cmd, "no-headers"))
 			checkErr(err)
 
-			printer, versioned, err := kubectl.GetPrinter(outputFormat, templateFile, defaultPrinter)
+			outputVersion := GetFlagString(cmd, "output-version")
+			if len(outputVersion) == 0 {
+				outputVersion = mapping.APIVersion
+			}
+			printer, err := kubectl.GetPrinter(outputVersion, outputFormat, templateFile, defaultPrinter)
 			checkErr(err)
 
 			obj, err := kubectl.NewRESTHelper(client, mapping).Get(namespace, name, labels)
 			checkErr(err)
 
-			if versioned {
-				outputVersion := GetFlagString(cmd, "output-version")
-				if len(outputVersion) == 0 {
-					outputVersion = mapping.APIVersion
-				}
-
-				obj, err = mapping.ObjectConvertor.ConvertToVersion(obj, outputVersion)
-				checkErr(err)
-			}
-
 			if err := printer.PrintObj(obj, out); err != nil {
 				checkErr(fmt.Errorf("Unable to output the provided object: %v", err))
 			}

pkg/kubectl/resource_printer.go

@@ -37,69 +37,108 @@ import (
 
 // GetPrinter returns a resource printer and a bool indicating whether the object must be
 // versioned for the given format.
-func GetPrinter(format, templateFile string, defaultPrinter ResourcePrinter) (ResourcePrinter, bool, error) {
+func GetPrinter(version, format, templateFile string, defaultPrinter ResourcePrinter) (ResourcePrinter, error) {
-	versioned := true
 	var printer ResourcePrinter
 	switch format {
 	case "json":
-		printer = &JSONPrinter{}
+		printer = &JSONPrinter{version}
 	case "yaml":
-		printer = &YAMLPrinter{}
+		printer = &YAMLPrinter{version}
 	case "template":
 		if len(templateFile) == 0 {
-			return nil, false, fmt.Errorf("template format specified but no template given")
+			return nil, fmt.Errorf("template format specified but no template given")
 		}
 		var err error
-		printer, err = NewTemplatePrinter([]byte(templateFile))
+		printer, err = NewTemplatePrinter(version, []byte(templateFile))
 		if err != nil {
-			return nil, false, fmt.Errorf("error parsing template %s, %v\n", templateFile, err)
+			return nil, fmt.Errorf("error parsing template %s, %v\n", templateFile, err)
 		}
 	case "templatefile":
 		if len(templateFile) == 0 {
-			return nil, false, fmt.Errorf("templatefile format specified but no template file given")
+			return nil, fmt.Errorf("templatefile format specified but no template file given")
 		}
 		data, err := ioutil.ReadFile(templateFile)
 		if err != nil {
-			return nil, false, fmt.Errorf("error reading template %s, %v\n", templateFile, err)
+			return nil, fmt.Errorf("error reading template %s, %v\n", templateFile, err)
 		}
-		printer, err = NewTemplatePrinter(data)
+		printer, err = NewTemplatePrinter(version, data)
 		if err != nil {
-			return nil, false, fmt.Errorf("error parsing template %s, %v\n", string(data), err)
+			return nil, fmt.Errorf("error parsing template %s, %v\n", string(data), err)
 		}
 	case "":
 		printer = defaultPrinter
-		versioned = false
 	default:
-		return nil, false, fmt.Errorf("output format %q not recognized", format)
+		return nil, fmt.Errorf("output format %q not recognized", format)
 	}
-	return printer, versioned, nil
+	return printer, nil
 }
 
 // ResourcePrinter is an interface that knows how to print API resources.
 type ResourcePrinter interface {
 	// Print receives an arbitrary object, formats it and prints it to a writer.
 	PrintObj(runtime.Object, io.Writer) error
+
+	// Returns true if this printer emits properly versioned output.
+	IsVersioned() bool
 }
 
-// IdentityPrinter is an implementation of ResourcePrinter which simply copies the body out to the output stream.
+// JSONPrinter is an implementation of ResourcePrinter which prints as JSON.
-type JSONPrinter struct{}
+type JSONPrinter struct {
+	version string
+}
 
 // PrintObj is an implementation of ResourcePrinter.PrintObj which simply writes the object to the Writer.
-func (i *JSONPrinter) PrintObj(obj runtime.Object, w io.Writer) error {
+func (j *JSONPrinter) PrintObj(obj runtime.Object, w io.Writer) error {
-	output, err := json.MarshalIndent(obj, "", "    ")
+	vi, err := latest.InterfacesFor(j.version)
 	if err != nil {
 		return err
 	}
-	_, err = fmt.Fprint(w, string(output)+"\n")
+
+	data, err := vi.Codec.Encode(obj)
+	if err != nil {
+		return err
+	}
+	dst := bytes.Buffer{}
+	err = json.Indent(&dst, data, "", "    ")
+	dst.WriteByte('\n')
+	_, err = w.Write(dst.Bytes())
 	return err
 }
 
-// YAMLPrinter is an implementation of ResourcePrinter which parsess JSON, and re-formats as YAML.
+// IsVersioned returns true.
-type YAMLPrinter struct{}
+func (*JSONPrinter) IsVersioned() bool { return true }
+
+func toVersionedMap(version string, obj runtime.Object) (map[string]interface{}, error) {
+	vi, err := latest.InterfacesFor(version)
+	if err != nil {
+		return nil, err
+	}
+
+	data, err := vi.Codec.Encode(obj)
+	if err != nil {
+		return nil, err
+	}
+	outObj := map[string]interface{}{}
+	err = json.Unmarshal(data, &outObj)
+	if err != nil {
+		return nil, err
+	}
+	return outObj, nil
+}
+
+// YAMLPrinter is an implementation of ResourcePrinter which prints as YAML.
+type YAMLPrinter struct {
+	version string
+}
 
 // PrintObj prints the data as YAML.
 func (y *YAMLPrinter) PrintObj(obj runtime.Object, w io.Writer) error {
-	output, err := yaml.Marshal(obj)
+	outObj, err := toVersionedMap(y.version, obj)
+	if err != nil {
+		return err
+	}
+
+	output, err := yaml.Marshal(outObj)
 	if err != nil {
 		return err
 	}
@@ -107,6 +146,9 @@ func (y *YAMLPrinter) PrintObj(obj runtime.Object, w io.Writer) error {
 	return err
 }
 
+// IsVersioned returns true.
+func (*YAMLPrinter) IsVersioned() bool { return true }
+
 type handlerEntry struct {
 	columns   []string
 	printFunc reflect.Value
@@ -118,6 +160,9 @@ type HumanReadablePrinter struct {
 	noHeaders  bool
 }
 
+// IsVersioned returns false-- human readable printers do not make versioned output.
+func (*HumanReadablePrinter) IsVersioned() bool { return false }
+
 // NewHumanReadablePrinter creates a HumanReadablePrinter.
 func NewHumanReadablePrinter(noHeaders bool) *HumanReadablePrinter {
 	printer := &HumanReadablePrinter{
@@ -315,25 +360,24 @@ func (h *HumanReadablePrinter) PrintObj(obj runtime.Object, output io.Writer) er
 
 // TemplatePrinter is an implementation of ResourcePrinter which formats data with a Go Template.
 type TemplatePrinter struct {
+	version  string
 	template *template.Template
 }
 
-func NewTemplatePrinter(tmpl []byte) (*TemplatePrinter, error) {
+func NewTemplatePrinter(version string, tmpl []byte) (*TemplatePrinter, error) {
 	t, err := template.New("output").Parse(string(tmpl))
 	if err != nil {
 		return nil, err
 	}
-	return &TemplatePrinter{t}, nil
+	return &TemplatePrinter{version, t}, nil
 }
 
+// IsVersioned returns true.
+func (*TemplatePrinter) IsVersioned() bool { return true }
+
 // PrintObj formats the obj with the Go Template.
 func (t *TemplatePrinter) PrintObj(obj runtime.Object, w io.Writer) error {
-	data, err := latest.Codec.Encode(obj)
+	outObj, err := toVersionedMap(t.version, obj)
-	if err != nil {
-		return err
-	}
-	outObj := map[string]interface{}{}
-	err = json.Unmarshal(data, &outObj)
 	if err != nil {
 		return err
 	}

pkg/kubectl/resource_printer_test.go

@@ -25,40 +25,52 @@ import (
 	"testing"
 
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/latest"
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
+
 	"gopkg.in/v1/yaml"
 )
 
 type testStruct struct {
-	Key        string         `yaml:"Key" json:"Key"`
+	api.TypeMeta   `yaml:",inline" json:",inline"`
-	Map        map[string]int `yaml:"Map" json:"Map"`
+	api.ObjectMeta `yaml:"metadata,omitempty" json:"metadata,omitempty"`
-	StringList []string       `yaml:"StringList" json:"StringList"`
+	Key            string         `yaml:"Key" json:"Key"`
-	IntList    []int          `yaml:"IntList" json:"IntList"`
+	Map            map[string]int `yaml:"Map" json:"Map"`
+	StringList     []string       `yaml:"StringList" json:"StringList"`
+	IntList        []int          `yaml:"IntList" json:"IntList"`
 }
 
 func (ts *testStruct) IsAnAPIObject() {}
 
+const TestVersion = latest.Version
+
+func init() {
+	api.Scheme.AddKnownTypes("", &testStruct{})
+	api.Scheme.AddKnownTypes(TestVersion, &testStruct{})
+}
+
 var testData = testStruct{
-	"testValue",
+	Key:        "testValue",
-	map[string]int{"TestSubkey": 1},
+	Map:        map[string]int{"TestSubkey": 1},
-	[]string{"a", "b", "c"},
+	StringList: []string{"a", "b", "c"},
-	[]int{1, 2, 3},
+	IntList:    []int{1, 2, 3},
 }
 
 func TestYAMLPrinter(t *testing.T) {
-	testPrinter(t, &YAMLPrinter{}, yaml.Unmarshal)
+	testPrinter(t, &YAMLPrinter{TestVersion}, yaml.Unmarshal)
 }
 
 func TestJSONPrinter(t *testing.T) {
-	testPrinter(t, &JSONPrinter{}, json.Unmarshal)
+	testPrinter(t, &JSONPrinter{TestVersion}, json.Unmarshal)
 }
 
 func TestPrintJSON(t *testing.T) {
 	buf := bytes.NewBuffer([]byte{})
-	printer, versioned, err := GetPrinter("json", "", nil)
+	printer, err := GetPrinter(TestVersion, "json", "", nil)
 	if err != nil {
 		t.Errorf("unexpected error: %#v", err)
 	}
-	if !versioned {
+	if !printer.IsVersioned() {
 		t.Errorf("printer should be versioned")
 	}
 	printer.PrintObj(&api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}}, buf)
@@ -70,11 +82,11 @@ func TestPrintJSON(t *testing.T) {
 
 func TestPrintYAML(t *testing.T) {
 	buf := bytes.NewBuffer([]byte{})
-	printer, versioned, err := GetPrinter("yaml", "", nil)
+	printer, err := GetPrinter(TestVersion, "yaml", "", nil)
 	if err != nil {
 		t.Errorf("unexpected error: %#v", err)
 	}
-	if !versioned {
+	if !printer.IsVersioned() {
 		t.Errorf("printer should be versioned")
 	}
 	printer.PrintObj(&api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}}, buf)
@@ -86,11 +98,11 @@ func TestPrintYAML(t *testing.T) {
 
 func TestPrintTemplate(t *testing.T) {
 	buf := bytes.NewBuffer([]byte{})
-	printer, versioned, err := GetPrinter("template", "{{.id}}", nil)
+	printer, err := GetPrinter(TestVersion, "template", "{{.id}}", nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %#v", err)
 	}
-	if !versioned {
+	if !printer.IsVersioned() {
 		t.Errorf("printer should be versioned")
 	}
 	err = printer.PrintObj(&api.Pod{ObjectMeta: api.ObjectMeta{Name: "foo"}}, buf)
@@ -103,19 +115,19 @@ func TestPrintTemplate(t *testing.T) {
 }
 
 func TestPrintEmptyTemplate(t *testing.T) {
-	if _, _, err := GetPrinter("template", "", nil); err == nil {
+	if _, err := GetPrinter(TestVersion, "template", "", nil); err == nil {
 		t.Errorf("unexpected non-error")
 	}
 }
 
 func TestPrintBadTemplate(t *testing.T) {
-	if _, _, err := GetPrinter("template", "{{ .Name", nil); err == nil {
+	if _, err := GetPrinter(TestVersion, "template", "{{ .Name", nil); err == nil {
 		t.Errorf("unexpected non-error")
 	}
 }
 
 func TestPrintBadTemplateFile(t *testing.T) {
-	if _, _, err := GetPrinter("templatefile", "", nil); err == nil {
+	if _, err := GetPrinter(TestVersion, "templatefile", "", nil); err == nil {
 		t.Errorf("unexpected non-error")
 	}
 }
@@ -128,12 +140,19 @@ func testPrinter(t *testing.T, printer ResourcePrinter, unmarshalFunc func(data
 		t.Fatal(err)
 	}
 	var poutput testStruct
-	err = yaml.Unmarshal(buf.Bytes(), &poutput)
+	// Verify that given function runs without error.
+	err = unmarshalFunc(buf.Bytes(), &poutput)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Use real decode function to undo the versioning process.
+	poutput = testStruct{}
+	err = latest.Codec.DecodeInto(buf.Bytes(), &poutput)
 	if err != nil {
 		t.Fatal(err)
 	}
 	if !reflect.DeepEqual(testData, poutput) {
-		t.Errorf("Test data and unmarshaled data are not equal: %#v vs %#v", poutput, testData)
+		t.Errorf("Test data and unmarshaled data are not equal: %v", util.ObjectDiff(poutput, testData))
 	}
 
 	obj := &api.Pod{
@@ -142,12 +161,19 @@ func testPrinter(t *testing.T, printer ResourcePrinter, unmarshalFunc func(data
 	buf.Reset()
 	printer.PrintObj(obj, buf)
 	var objOut api.Pod
-	err = yaml.Unmarshal([]byte(buf.String()), &objOut)
+	// Verify that given function runs without error.
+	err = unmarshalFunc(buf.Bytes(), &objOut)
 	if err != nil {
 		t.Errorf("Unexpeted error: %#v", err)
 	}
+	// Use real decode function to undo the versioning process.
+	objOut = api.Pod{}
+	err = latest.Codec.DecodeInto(buf.Bytes(), &objOut)
+	if err != nil {
+		t.Fatal(err)
+	}
 	if !reflect.DeepEqual(obj, &objOut) {
-		t.Errorf("Unexpected inequality:\n%#v \nvs\n%#v", obj, &objOut)
+		t.Errorf("Unexpected inequality:\n%v", util.ObjectDiff(obj, &objOut))
 	}
 }
 
@@ -210,7 +236,7 @@ func TestUnknownTypePrinting(t *testing.T) {
 
 func TestTemplateEmitsVersionedObjects(t *testing.T) {
 	// kind is always blank in memory and set on the wire
-	printer, err := NewTemplatePrinter([]byte(`{{.kind}}`))
+	printer, err := NewTemplatePrinter(TestVersion, []byte(`{{.kind}}`))
 	if err != nil {
 		t.Fatalf("tmpl fail: %v", err)
 	}

pkg/registry/event/rest.go

@@ -88,8 +88,10 @@ func (rs *REST) getAttrs(obj runtime.Object) (objLabels, objFields labels.Set, e
 	if !ok {
 		return nil, nil, fmt.Errorf("invalid object type")
 	}
+	// TODO: internal version leaks through here. This should be versioned.
 	return labels.Set{}, labels.Set{
 		"involvedObject.kind":            event.InvolvedObject.Kind,
+		"involvedObject.namespace":       event.InvolvedObject.Namespace,
 		"involvedObject.name":            event.InvolvedObject.Name,
 		"involvedObject.uid":             event.InvolvedObject.UID,
 		"involvedObject.apiVersion":      event.InvolvedObject.APIVersion,
@@ -97,6 +99,7 @@ func (rs *REST) getAttrs(obj runtime.Object) (objLabels, objFields labels.Set, e
 		"involvedObject.fieldPath":       event.InvolvedObject.FieldPath,
 		"status":                         event.Status,
 		"reason":                         event.Reason,
+		"source":                         event.Source,
 	}, nil
 }
 

pkg/registry/event/rest_test.go

@@ -98,6 +98,7 @@ func TestRESTgetAttrs(t *testing.T) {
 		InvolvedObject: api.ObjectReference{
 			Kind:            "Pod",
 			Name:            "foo",
+			Namespace:       "baz",
 			UID:             "long uid string",
 			APIVersion:      testapi.Version(),
 			ResourceVersion: "0",
@@ -105,6 +106,7 @@ func TestRESTgetAttrs(t *testing.T) {
 		},
 		Status: "tested",
 		Reason: "forTesting",
+		Source: "test",
 	}
 	label, field, err := rest.getAttrs(eventA)
 	if err != nil {
@@ -116,12 +118,14 @@ func TestRESTgetAttrs(t *testing.T) {
 	expect := labels.Set{
 		"involvedObject.kind":            "Pod",
 		"involvedObject.name":            "foo",
+		"involvedObject.namespace":       "baz",
 		"involvedObject.uid":             "long uid string",
 		"involvedObject.apiVersion":      testapi.Version(),
 		"involvedObject.resourceVersion": "0",
 		"involvedObject.fieldPath":       "",
 		"status":                         "tested",
 		"reason":                         "forTesting",
+		"source":                         "test",
 	}
 	if e, a := expect, field; !reflect.DeepEqual(e, a) {
 		t.Errorf("diff: %s", util.ObjectDiff(e, a))