Add custom conversion function system.

As an example, demonstrate how Env.Key's deprecation ought to work.

pkg/api/apiobj_test.go

@@ -28,11 +28,8 @@ func TestAPIObject(t *testing.T) {
 		Object      APIObject `yaml:"object,omitempty" json:"object,omitempty"`
 		EmptyObject APIObject `yaml:"emptyObject,omitempty" json:"emptyObject,omitempty"`
 	}
-	convert := func(obj interface{}) (interface{}, error) { return obj, nil }
 	AddKnownTypes("", EmbeddedTest{})
 	AddKnownTypes("v1beta1", EmbeddedTest{})
-	AddExternalConversion("EmbeddedTest", convert)
-	AddInternalConversion("EmbeddedTest", convert)
 
 	outer := &EmbeddedTest{
 		JSONBase: JSONBase{ID: "outer"},

pkg/api/converter.go

@@ -0,0 +1,195 @@
+/*
+Copyright 2014 Google Inc. All rights reserved.
+
+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 api
+
+import (
+	"fmt"
+	"reflect"
+)
+
+type typePair struct {
+	source reflect.Type
+	dest   reflect.Type
+}
+
+type debugLogger interface {
+	Logf(format string, args ...interface{})
+}
+
+// Converter knows how to convert one type to another.
+type Converter struct {
+	// Map from the conversion pair to a function which can
+	// do the conversion.
+	funcs map[typePair]reflect.Value
+
+	// If true, print helpful debugging info. Quite verbose.
+	debug debugLogger
+}
+
+// NewConverter makes a new Converter object.
+func NewConverter() *Converter {
+	return &Converter{
+		funcs: map[typePair]reflect.Value{},
+	}
+}
+
+// Register registers a conversion func with the Converter. conversionFunc must take
+// two parameters, the input and output type. It must take a pointer to each. It must
+// return an error.
+//
+// Example:
+// c.Register(func(in *Pod, out *v1beta1.Pod) error { ... return nil })
+func (c *Converter) Register(conversionFunc interface{}) error {
+	fv := reflect.ValueOf(conversionFunc)
+	ft := fv.Type()
+	if ft.Kind() != reflect.Func {
+		return fmt.Errorf("expected func, got: %v", ft)
+	}
+	if ft.NumIn() != 2 {
+		return fmt.Errorf("expected two in params, got: %v", ft)
+	}
+	if ft.NumOut() != 1 {
+		return fmt.Errorf("expected one out param, got: %v", ft)
+	}
+	if ft.In(0).Kind() != reflect.Ptr {
+		return fmt.Errorf("expected pointer arg for in param 0, got: %v", ft)
+	}
+	if ft.In(1).Kind() != reflect.Ptr {
+		return fmt.Errorf("expected pointer arg for in param 1, got: %v", ft)
+	}
+	var forErrorType error
+	// This convolution is necessary, otherwise TypeOf picks up on the fact
+	// that forErrorType is nil.
+	errorType := reflect.TypeOf(&forErrorType).Elem()
+	if ft.Out(0) != errorType {
+		return fmt.Errorf("expected error return, got: %v", ft)
+	}
+	c.funcs[typePair{ft.In(0).Elem(), ft.In(1).Elem()}] = fv
+	return nil
+}
+
+// Convert will translate src to dest if it knows how. Both must be pointers.
+// If no conversion func is registered and the default copying mechanism
+// doesn't work on this type pair, an error will be returned.
+// Not safe for objects with cyclic references!
+func (c *Converter) Convert(src, dest interface{}) error {
+	dv, sv := reflect.ValueOf(dest), reflect.ValueOf(src)
+	if dv.Kind() != reflect.Ptr {
+		return fmt.Errorf("Need pointer, but got %#v", dest)
+	}
+	if sv.Kind() != reflect.Ptr {
+		return fmt.Errorf("Need pointer, but got %#v", src)
+	}
+	dv = dv.Elem()
+	sv = sv.Elem()
+	if !dv.CanAddr() {
+		return fmt.Errorf("Can't write to dest")
+	}
+	return c.convert(sv, dv)
+}
+
+// convert recursively copies sv into dv, calling an appropriate conversion function if
+// one is registered.
+func (c *Converter) convert(sv, dv reflect.Value) error {
+	dt, st := dv.Type(), sv.Type()
+	if fv, ok := c.funcs[typePair{st, dt}]; ok {
+		if c.debug != nil {
+			c.debug.Logf("Calling custom conversion of '%v' to '%v'", st, dt)
+		}
+		ret := fv.Call([]reflect.Value{sv.Addr(), dv.Addr()})[0].Interface()
+		// This convolution is necssary because nil interfaces won't convert
+		// to errors.
+		if ret == nil {
+			return nil
+		}
+		return ret.(error)
+	}
+
+	if dt.Name() != st.Name() {
+		return fmt.Errorf("Type names don't match: %v, %v", dt.Name(), st.Name())
+	}
+
+	// This should handle all simple types.
+	if st.AssignableTo(dt) {
+		dv.Set(sv)
+		return nil
+	}
+	if st.ConvertibleTo(dt) {
+		dv.Set(sv.Convert(dt))
+		return nil
+	}
+
+	if c.debug != nil {
+		c.debug.Logf("Trying to convert '%v' to '%v'", st, dt)
+	}
+
+	switch dv.Kind() {
+	case reflect.Struct:
+		for i := 0; i < dt.NumField(); i++ {
+			f := dv.Type().Field(i)
+			df := dv.FieldByName(f.Name)
+			sf := sv.FieldByName(f.Name)
+			if !df.IsValid() || !sf.IsValid() {
+				return fmt.Errorf("%v not present in source and dest.", f.Name)
+			}
+			if err := c.convert(sf, df); err != nil {
+				return err
+			}
+		}
+	case reflect.Slice:
+		if sv.IsNil() {
+			// Don't make a zero-length slice.
+			dv.Set(reflect.Zero(dt))
+			return nil
+		}
+		dv.Set(reflect.MakeSlice(dt, sv.Len(), sv.Cap()))
+		for i := 0; i < sv.Len(); i++ {
+			if err := c.convert(sv.Index(i), dv.Index(i)); err != nil {
+				return err
+			}
+		}
+	case reflect.Ptr:
+		if sv.IsNil() {
+			// Don't copy a nil ptr!
+			dv.Set(reflect.Zero(dt))
+			return nil
+		}
+		dv.Set(reflect.New(dt.Elem()))
+		return c.convert(sv.Elem(), dv.Elem())
+	case reflect.Map:
+		if sv.IsNil() {
+			// Don't copy a nil ptr!
+			dv.Set(reflect.Zero(dt))
+			return nil
+		}
+		dv.Set(reflect.MakeMap(dt))
+		for _, sk := range sv.MapKeys() {
+			dk := reflect.New(dt.Key()).Elem()
+			if err := c.convert(sk, dk); err != nil {
+				return err
+			}
+			dkv := reflect.New(dt.Elem()).Elem()
+			if err := c.convert(sv.MapIndex(sk), dkv); err != nil {
+				return err
+			}
+			dv.SetMapIndex(dk, dkv)
+		}
+	default:
+		return fmt.Errorf("Couldn't copy '%v' into '%v'", st, dt)
+	}
+	return nil
+}

pkg/api/converter_test.go

@@ -0,0 +1,81 @@
+/*
+Copyright 2014 Google Inc. All rights reserved.
+
+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 api
+
+import (
+	"fmt"
+	"testing"
+)
+
+func TestConverter(t *testing.T) {
+	type A struct {
+		Foo string
+	}
+	type B struct {
+		Bar string
+	}
+	type C struct{}
+	c := NewConverter()
+	err := c.Register(func(in *A, out *B) error {
+		out.Bar = in.Foo
+		return nil
+	})
+	if err != nil {
+		t.Fatalf("unexpected error %v", err)
+	}
+	err = c.Register(func(in *B, out *A) error {
+		out.Foo = in.Bar
+		return nil
+	})
+	if err != nil {
+		t.Fatalf("unexpected error %v", err)
+	}
+
+	x := A{"hello, intrepid test reader!"}
+	y := B{}
+
+	err = c.Convert(&x, &y)
+	if err != nil {
+		t.Fatalf("unexpected error %v", err)
+	}
+	if e, a := x.Foo, y.Bar; e != a {
+		t.Errorf("expected %v, got %v", e, a)
+	}
+
+	z := B{"all your test are belong to us"}
+	w := A{}
+
+	err = c.Convert(&z, &w)
+	if err != nil {
+		t.Fatalf("unexpected error %v", err)
+	}
+	if e, a := z.Bar, w.Foo; e != a {
+		t.Errorf("expected %v, got %v", e, a)
+	}
+
+	err = c.Register(func(in *A, out *C) error {
+		return fmt.Errorf("C can't store an A, silly")
+	})
+	if err != nil {
+		t.Fatalf("unexpected error %v", err)
+	}
+
+	err = c.Convert(&A{}, &C{})
+	if err == nil {
+		t.Errorf("unexpected non-error")
+	}
+}

pkg/api/defaultcopy.go

@@ -1,133 +0,0 @@
-/*
-Copyright 2014 Google Inc. All rights reserved.
-
-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 api
-
-import (
-	"fmt"
-	"reflect"
-)
-
-// DefaultCopy copies API objects to/from their corresponding types
-// in a versioned package (e.g., v1beta1). Only suitable for types
-// in which no fields changed.
-// dest and src must both be pointers to API objects.
-// Not safe for objects with cyclic references!
-// TODO: Allow overrides, using the same function mechanism that
-// util.Fuzzer allows.
-func DefaultCopy(src, dest interface{}) error {
-	dv, sv := reflect.ValueOf(dest), reflect.ValueOf(src)
-	if dv.Kind() != reflect.Ptr {
-		return fmt.Errorf("Need pointer, but got %#v", dest)
-	}
-	if sv.Kind() != reflect.Ptr {
-		return fmt.Errorf("Need pointer, but got %#v", src)
-	}
-	dv = dv.Elem()
-	sv = sv.Elem()
-	if !dv.CanAddr() {
-		return fmt.Errorf("Can't write to dest")
-	}
-
-	// Ensure there's no reversed src/dest bugs by making src unwriteable.
-	sv = reflect.ValueOf(sv.Interface())
-	if sv.CanAddr() {
-		return fmt.Errorf("Can write to src, shouldn't be able to.")
-	}
-
-	return copyValue(sv, dv)
-}
-
-// Recursively copy sv into dv
-func copyValue(sv, dv reflect.Value) error {
-	dt, st := dv.Type(), sv.Type()
-	if dt.Name() != st.Name() {
-		return fmt.Errorf("Type names don't match: %v, %v", dt.Name(), st.Name())
-	}
-
-	// This should handle all simple types.
-	if st.AssignableTo(dt) {
-		dv.Set(sv)
-		return nil
-	} else if st.ConvertibleTo(dt) {
-		dv.Set(sv.Convert(dt))
-		return nil
-	}
-
-	// For debugging, should you need to do that.
-	if false {
-		fmt.Printf("copyVal of %v.%v (%v) -> %v.%v (%v)\n",
-			st.PkgPath(), st.Name(), st.Kind(),
-			dt.PkgPath(), dt.Name(), dt.Kind())
-	}
-
-	switch dv.Kind() {
-	case reflect.Struct:
-		for i := 0; i < dt.NumField(); i++ {
-			f := dv.Type().Field(i)
-			df := dv.FieldByName(f.Name)
-			sf := sv.FieldByName(f.Name)
-			if !df.IsValid() || !sf.IsValid() {
-				return fmt.Errorf("%v not present in source and dest.", f.Name)
-			}
-			if err := copyValue(sf, df); err != nil {
-				return err
-			}
-		}
-	case reflect.Slice:
-		if sv.IsNil() {
-			// Don't make a zero-length slice.
-			dv.Set(reflect.Zero(dt))
-			return nil
-		}
-		dv.Set(reflect.MakeSlice(dt, sv.Len(), sv.Cap()))
-		for i := 0; i < sv.Len(); i++ {
-			if err := copyValue(sv.Index(i), dv.Index(i)); err != nil {
-				return err
-			}
-		}
-	case reflect.Ptr:
-		if sv.IsNil() {
-			// Don't copy a nil ptr!
-			dv.Set(reflect.Zero(dt))
-			return nil
-		}
-		dv.Set(reflect.New(dt.Elem()))
-		return copyValue(sv.Elem(), dv.Elem())
-	case reflect.Map:
-		if sv.IsNil() {
-			// Don't copy a nil ptr!
-			dv.Set(reflect.Zero(dt))
-			return nil
-		}
-		dv.Set(reflect.MakeMap(dt))
-		for _, sk := range sv.MapKeys() {
-			dk := reflect.New(dt.Key()).Elem()
-			if err := copyValue(sk, dk); err != nil {
-				return err
-			}
-			dkv := reflect.New(dt.Elem()).Elem()
-			if err := copyValue(sv.MapIndex(sk), dkv); err != nil {
-				return err
-			}
-			dv.SetMapIndex(dk, dkv)
-		}
-	default:
-		return fmt.Errorf("Couldn't copy %#v (%v) into %#v (%v)",
-			sv.Interface(), sv.Kind(), dv.Interface(), dv.Kind())
-	}
-	return nil
-}

pkg/api/helper.go

@@ -25,33 +25,18 @@ import (
 	"gopkg.in/v1/yaml"
 )
 
+// versionMap allows one to figure out the go type of an object with
+// the given version and name.
 var versionMap = map[string]map[string]reflect.Type{}
 
-// typeNamePath records go's name and path of a go struct.
-type typeNamePath struct {
-	typeName string
-	typePath string
-}
+// typeToVersion allows one to figure out the version for a given go object.
+// The reflect.Type we index by should *not* be a pointer. If the same type
+// is registered for multiple versions, the last one wins.
+var typeToVersion = map[reflect.Type]string{}
 
-// typeNamePathToVersion allows one to figure out the version for a
-// given go object.
-var typeNamePathToVersion = map[typeNamePath]string{}
-
-// ConversionFunc knows how to translate a type from one api version to another.
-type ConversionFunc func(input interface{}) (output interface{}, err error)
-
-// typeTuple indexes a conversionFunc by source and dest version, and
-// the name of the type it operates on.
-type typeTuple struct {
-	sourceVersion string
-	destVersion   string
-
-	// Go name of this type.
-	typeName string
-}
-
-// conversionFuncs is a map of all known conversion functions.
-var conversionFuncs = map[typeTuple]ConversionFunc{}
+// theConverter stores all registered conversion functions. It also has
+// default coverting behavior.
+var theConverter = NewConverter()
 
 func init() {
 	AddKnownTypes("",
@@ -85,24 +70,29 @@ func init() {
 		v1beta1.Endpoints{},
 	)
 
-	defaultCopyList := []string{
-		"PodList",
-		"Pod",
-		"ReplicationControllerList",
-		"ReplicationController",
-		"ServiceList",
-		"Service",
-		"MinionList",
-		"Minion",
-		"Status",
-		"ServerOpList",
-		"ServerOp",
-		"ContainerManifestList",
-		"Endpoints",
-	}
-
-	AddDefaultCopy("", "v1beta1", defaultCopyList...)
-	AddDefaultCopy("v1beta1", "", defaultCopyList...)
+	// TODO: when we get more of this stuff, move to its own file. This is not a
+	// good home for lots of conversion functions.
+	// TODO: Consider inverting dependency chain-- imagine v1beta1 package
+	// registering all of these functions. Then, if you want to be able to understand
+	// v1beta1 objects, you just import that package for its side effects.
+	AddConversionFuncs(
+		// EnvVar's Name is depricated in favor of Key.
+		func(in *EnvVar, out *v1beta1.EnvVar) error {
+			out.Value = in.Value
+			out.Key = in.Name
+			out.Name = in.Name
+			return nil
+		},
+		func(in *v1beta1.EnvVar, out *EnvVar) error {
+			out.Value = in.Value
+			if in.Name != "" {
+				out.Name = in.Name
+			} else {
+				out.Name = in.Key
+			}
+			return nil
+		},
+	)
 }
 
 // AddKnownTypes registers the types of the arguments to the marshaller of the package api.
@@ -119,10 +109,7 @@ func AddKnownTypes(version string, types ...interface{}) {
 			panic("All types must be structs.")
 		}
 		knownTypes[t.Name()] = t
-		typeNamePathToVersion[typeNamePath{
-			typeName: t.Name(),
-			typePath: t.PkgPath(),
-		}] = version
+		typeToVersion[t] = version
 	}
 }
 
@@ -138,41 +125,33 @@ func New(versionName, typeName string) (interface{}, error) {
 	return nil, fmt.Errorf("No version '%v'", versionName)
 }
 
-// AddExternalConversion adds a function to the list of conversion functions. The given
-// function should know how to convert the internal representation of 'typeName' to the
-// external, versioned representation ("v1beta1").
-// TODO: When we make the next api version, this function will have to add a destination
-// version parameter.
-func AddExternalConversion(typeName string, fn ConversionFunc) {
-	conversionFuncs[typeTuple{"", "v1beta1", typeName}] = fn
-}
-
-// AddInternalConversion adds a function to the list of conversion functions. The given
-// function should know how to convert the external, versioned representation of 'typeName'
-// to the internal representation.
-// TODO: When we make the next api version, this function will have to add a source
-// version parameter.
-func AddInternalConversion(typeName string, fn ConversionFunc) {
-	conversionFuncs[typeTuple{"v1beta1", "", typeName}] = fn
-}
-
-// AddDefaultCopy registers a general copying function for turning objects of version
-// sourceVersion into the same object of version destVersion.
-func AddDefaultCopy(sourceVersion, destVersion string, types ...string) {
-	for i := range types {
-		t := types[i]
-		conversionFuncs[typeTuple{sourceVersion, destVersion, t}] = func(in interface{}) (interface{}, error) {
-			out, err := New(destVersion, t)
-			if err != nil {
-				return nil, err
-			}
-			err = DefaultCopy(in, out)
-			if err != nil {
-				return nil, err
-			}
-			return out, nil
+// AddConversionFuncs adds a function to the list of conversion functions. The given
+// function should know how to convert between two API objects. We deduce how to call
+// it from the types of its two parameters; see the comment for Converter.Register.
+//
+// Note that, if you need to copy sub-objects that didn't change, it's safe to call
+// Convert() inside your conversionFuncs, as long as you don't start a conversion
+// chain that's infinitely recursive.
+//
+// Also note that the default behavior, if you don't add a conversion function, is to
+// sanely copy fields that have the same names. It's OK if the destination type has
+// extra fields, but it must not remove any. So you only need to add a conversion
+// function for things with changed/removed fields.
+func AddConversionFuncs(conversionFuncs ...interface{}) error {
+	for _, f := range conversionFuncs {
+		err := theConverter.Register(f)
+		if err != nil {
+			return err
 		}
 	}
+	return nil
+}
+
+// Convert will attempt to convert in into out. Both must be pointers to API objects.
+// For easy testing of conversion functions. Returns an error if the conversion isn't
+// possible.
+func Convert(in, out interface{}) error {
+	return theConverter.Convert(in, out)
 }
 
 // FindJSONBase takes an arbitary api type, returns pointer to its JSONBase field.
@@ -288,12 +267,8 @@ func objAPIVersionAndName(obj interface{}) (apiVersion, name string, err error)
 		return "", "", err
 	}
 	t := v.Type()
-	key := typeNamePath{
-		typeName: t.Name(),
-		typePath: t.PkgPath(),
-	}
-	if version, ok := typeNamePathToVersion[key]; !ok {
-		return "", "", fmt.Errorf("Unregistered type: %#v", key)
+	if version, ok := typeToVersion[t]; !ok {
+		return "", "", fmt.Errorf("Unregistered type: %v", t)
 	} else {
 		return version, t.Name(), nil
 	}
@@ -490,25 +465,33 @@ func DecodeInto(data []byte, obj interface{}) error {
 }
 
 func internalize(obj interface{}) (interface{}, error) {
-	objVersion, objKind, err := objAPIVersionAndName(obj)
+	_, objKind, err := objAPIVersionAndName(obj)
 	if err != nil {
 		return nil, err
 	}
-	if fn, ok := conversionFuncs[typeTuple{objVersion, "", objKind}]; ok {
-		return fn(obj)
+	objOut, err := New("", objKind)
+	if err != nil {
+		return nil, err
 	}
-	return nil, fmt.Errorf("No conversion handler that knows how to convert a '%v' from '%v'",
-		objKind, objVersion)
+	err = theConverter.Convert(obj, objOut)
+	if err != nil {
+		return nil, err
+	}
+	return objOut, nil
 }
 
 func externalize(obj interface{}) (interface{}, error) {
-	objVersion, objKind, err := objAPIVersionAndName(obj)
+	_, objKind, err := objAPIVersionAndName(obj)
 	if err != nil {
 		return nil, err
 	}
-	if fn, ok := conversionFuncs[typeTuple{objVersion, "v1beta1", objKind}]; ok {
-		return fn(obj)
+	objOut, err := New("v1beta1", objKind)
+	if err != nil {
+		return nil, err
 	}
-	return nil, fmt.Errorf("No conversion handler that knows how to convert a '%v' from '%v' to '%v'",
-		objKind, objVersion, "v1beta1")
+	err = theConverter.Convert(obj, objOut)
+	if err != nil {
+		return nil, err
+	}
+	return objOut, nil
 }

pkg/api/types.go

@@ -129,10 +129,7 @@ type VolumeMount struct {
 // EnvVar represents an environment variable present in a Container
 type EnvVar struct {
 	// Required: This must be a C_IDENTIFIER.
-	// Exactly one of the following must be set.  If both are set, prefer Name.
-	// DEPRECATED: EnvVar.Key will be removed in a future version of the API.
 	Name string `yaml:"name" json:"name"`
-	Key  string `yaml:"key,omitempty" json:"key,omitempty"`
 	// Optional: defaults to "".
 	Value string `yaml:"value,omitempty" json:"value,omitempty"`
 }

pkg/api/validation.go

@@ -161,14 +161,7 @@ func validateEnv(vars []EnvVar) errorList {
 	for i := range vars {
 		ev := &vars[i] // so we can set default values
 		if len(ev.Name) == 0 {
-			// Backwards compat.
-			if len(ev.Key) == 0 {
-				allErrs.Append(makeInvalidError("EnvVar.Name", ev.Name))
-			} else {
-				glog.Warning("DEPRECATED: EnvVar.Key has been replaced by EnvVar.Name")
-				ev.Name = ev.Key
-				ev.Key = ""
-			}
+			allErrs.Append(makeInvalidError("EnvVar.Name", ev.Name))
 		}
 		if !util.IsCIdentifier(ev.Name) {
 			allErrs.Append(makeInvalidError("EnvVar.Name", ev.Name))

pkg/api/validation_test.go

@@ -17,9 +17,11 @@ limitations under the License.
 package api
 
 import (
+	"reflect"
 	"strings"
 	"testing"
 
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/api/v1beta1"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
@@ -105,16 +107,6 @@ func TestValidateEnv(t *testing.T) {
 		t.Errorf("expected success: %v", errs)
 	}
 
-	nonCanonicalCase := []EnvVar{
-		{Key: "EV"},
-	}
-	if errs := validateEnv(nonCanonicalCase); len(errs) != 0 {
-		t.Errorf("expected success: %v", errs)
-	}
-	if nonCanonicalCase[0].Name != "EV" || nonCanonicalCase[0].Value != "" {
-		t.Errorf("expected default values: %+v", nonCanonicalCase[0])
-	}
-
 	errorCases := map[string][]EnvVar{
 		"zero-length name":        {{Name: ""}},
 		"name not a C identifier": {{Name: "a.b.c"}},
@@ -126,6 +118,27 @@ func TestValidateEnv(t *testing.T) {
 	}
 }
 
+func TestEnvConversion(t *testing.T) {
+	nonCanonical := []v1beta1.EnvVar{
+		{Key: "EV"},
+		{Key: "EV", Name: "EX"},
+	}
+	cannonical := []EnvVar{
+		{Name: "EV"},
+		{Name: "EX"},
+	}
+	for i := range nonCanonical {
+		var got EnvVar
+		err := Convert(&nonCanonical[i], &got)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if e, a := cannonical[i], got; !reflect.DeepEqual(e, a) {
+			t.Errorf("expected %v, got %v", e, a)
+		}
+	}
+}
+
 func TestValidateVolumeMounts(t *testing.T) {
 	volumes := util.NewStringSet("abc", "123", "abc-123")
 
@@ -225,7 +238,7 @@ func TestValidateManifest(t *testing.T) {
 					Env: []EnvVar{
 						{Name: "ev1", Value: "val1"},
 						{Name: "ev2", Value: "val2"},
-						{Key: "EV3", Value: "val3"},
+						{Name: "EV3", Value: "val3"},
 					},
 					VolumeMounts: []VolumeMount{
 						{Name: "vol1", MountPath: "/foo"},

pkg/apiserver/apiserver_test.go

@@ -44,10 +44,6 @@ func convert(obj interface{}) (interface{}, error) {
 func init() {
 	api.AddKnownTypes("", Simple{}, SimpleList{})
 	api.AddKnownTypes("v1beta1", Simple{}, SimpleList{})
-	api.AddExternalConversion("Simple", convert)
-	api.AddInternalConversion("Simple", convert)
-	api.AddExternalConversion("SimpleList", convert)
-	api.AddInternalConversion("SimpleList", convert)
 }
 
 // TODO: This doesn't reduce typing enough to make it worth the less readable errors. Remove.