Small refactorings for kubectl/apply merge packages

- move strings into constants
- remove unnecessary interface
- fix documentation
- improve error messaging

pkg/kubectl/apply/element.go

@@ -78,16 +78,14 @@ type FieldMeta interface {
 
 // FieldMetaImpl implements FieldMeta
 type FieldMetaImpl struct {
-	// The type of merge strategy to use for this field
+	// MergeType is the type of merge strategy to use for this field
 	// maybe "merge", "replace" or "retainkeys"
-	// TODO: There maybe multiple strategies, so this may need to be a slice, map, or struct
-	// Address this in a follow up in the PR to introduce retainkeys strategy
 	MergeType string
 
-	// The merge key to use when the MergeType is "merge" and underlying type is a list
+	// MergeKeys are the merge keys to use when the MergeType is "merge" and underlying type is a list
 	MergeKeys MergeKeys
 
-	// The openapi type of the field - "list", "primitive", "map"
+	// Type is the openapi type of the field - "list", "primitive", "map"
 	Type string
 
 	// Name contains of the field

pkg/kubectl/apply/parse/factory.go

@@ -78,13 +78,13 @@ func (v *ElementBuildingVisitor) getItem(s proto.Schema, name string, data apply
 		reflect.String:
 		p, err := getPrimitive(s)
 		if err != nil {
-			return nil, fmt.Errorf("expected openapi Primitive, was %T for %v", s, kind)
+			return nil, fmt.Errorf("expected openapi Primitive, was %T for %v (%v)", s, kind, err)
 		}
 		return &primitiveItem{name, p, data}, nil
 	case reflect.Array, reflect.Slice:
 		a, err := getArray(s)
 		if err != nil {
-			return nil, fmt.Errorf("expected openapi Array, was %T for %v", s, kind)
+			return nil, fmt.Errorf("expected openapi Array, was %T for %v (%v)", s, kind, err)
 		}
 		return &listItem{
 			Name:  name,
@@ -106,7 +106,7 @@ func (v *ElementBuildingVisitor) getItem(s proto.Schema, name string, data apply
 		// If it looks like a map, and no openapi type is found, default to mapItem
 		m, err := getMap(s)
 		if err != nil {
-			return nil, fmt.Errorf("expected openapi Kind or Map, was %T for %v", s, kind)
+			return nil, fmt.Errorf("expected openapi Kind or Map, was %T for %v (%v)", s, kind, err)
 		}
 		return &mapItem{
 			Name: name,

pkg/kubectl/apply/parse/list_element.go

@@ -45,7 +45,7 @@ func (v ElementBuildingVisitor) mergeListElement(meta apply.FieldMetaImpl, item
 func (v ElementBuildingVisitor) doPrimitiveList(meta apply.FieldMetaImpl, item *listItem) (*apply.ListElement, error) {
 	result := &apply.ListElement{
 		FieldMetaImpl: apply.FieldMetaImpl{
-			MergeType: "merge",
+			MergeType: apply.MergeStrategy,
 			Name:      item.Name,
 		},
 		ListElementData: item.ListElementData,
@@ -101,7 +101,7 @@ func (v ElementBuildingVisitor) doMapList(meta apply.FieldMetaImpl, item *listIt
 	key := meta.GetFieldMergeKeys()
 	result := &apply.ListElement{
 		FieldMetaImpl: apply.FieldMetaImpl{
-			MergeType: "merge",
+			MergeType: apply.MergeStrategy,
 			MergeKeys: key,
 			Name:      item.Name,
 		},

pkg/kubectl/apply/parse/visitor.go

@@ -54,7 +54,7 @@ func (v ElementBuildingVisitor) CreateListElement(item *listItem) (apply.Element
 	if err != nil {
 		return nil, err
 	}
-	if meta.GetFieldMergeType() == "merge" {
+	if meta.GetFieldMergeType() == apply.MergeStrategy {
 		return v.mergeListElement(meta, item)
 	}
 	return v.replaceListElement(meta, item)

pkg/kubectl/apply/strategy/merge_visitor.go

@@ -29,13 +29,13 @@ func createMergeStrategy(options Options, strategic *delegatingStrategy) mergeSt
 	}
 }
 
-// mergeStrategy creates a patch to merge a local file value into a remote field value
+// mergeStrategy merges the values in an Element into a single Result
 type mergeStrategy struct {
 	strategic *delegatingStrategy
 	options   Options
 }
 
-// MergeList creates a patch to merge a local list field value into a remote list field value
+// MergeList merges the lists in a ListElement into a single Result
 func (v mergeStrategy) MergeList(e apply.ListElement) (apply.Result, error) {
 	// No merge logic if adding or deleting a field
 	if result, done := v.doAddOrDelete(e); done {
@@ -70,26 +70,32 @@ func (v mergeStrategy) MergeList(e apply.ListElement) (apply.Result, error) {
 	return apply.Result{Operation: apply.SET, MergedResult: merged}, nil
 }
 
-// MergeMap creates a patch to merge a local map field into a remote map field
+// MergeMap merges the maps in a MapElement into a single Result
 func (v mergeStrategy) MergeMap(e apply.MapElement) (apply.Result, error) {
-	return v.doMergeMap(e)
-}
-
-// MergeType creates a patch to merge a local map field into a remote map field
-func (v mergeStrategy) MergeType(e apply.TypeElement) (apply.Result, error) {
-	return v.doMergeMap(e)
-}
-
-// do merges a recorded, local and remote map into a new object
-func (v mergeStrategy) doMergeMap(e apply.MapValuesElement) (apply.Result, error) {
 	// No merge logic if adding or deleting a field
 	if result, done := v.doAddOrDelete(e); done {
 		return result, nil
 	}
 
+	return v.doMergeMap(e.GetValues())
+}
+
+// MergeMap merges the type instances in a TypeElement into a single Result
+func (v mergeStrategy) MergeType(e apply.TypeElement) (apply.Result, error) {
+	// No merge logic if adding or deleting a field
+	if result, done := v.doAddOrDelete(e); done {
+		return result, nil
+	}
+
+	return v.doMergeMap(e.GetValues())
+}
+
+// do merges a recorded, local and remote map into a new object
+func (v mergeStrategy) doMergeMap(e map[string]apply.Element) (apply.Result, error) {
+
 	// Merge each item in the list
 	merged := map[string]interface{}{}
-	for key, value := range e.GetValues() {
+	for key, value := range e {
 		// Recursively merge the map element before adding the value to the map
 		result, err := value.Merge(v.strategic)
 		if err != nil {
@@ -134,7 +140,7 @@ func (v mergeStrategy) MergePrimitive(diff apply.PrimitiveElement) (apply.Result
 	return apply.Result{}, fmt.Errorf("Cannot merge primitive element %v", diff.Name)
 }
 
-// MergeEmpty
+// MergeEmpty returns an empty result
 func (v mergeStrategy) MergeEmpty(diff apply.EmptyElement) (apply.Result, error) {
 	return apply.Result{Operation: apply.SET}, nil
 }

pkg/kubectl/apply/strategy/strategic_visitor.go

@@ -16,7 +16,9 @@ limitations under the License.
 
 package strategy
 
-import "k8s.io/kubernetes/pkg/kubectl/apply"
+import (
+	"k8s.io/kubernetes/pkg/kubectl/apply"
+)
 
 // delegatingStrategy delegates merging fields to other visitor implementations
 // based on the merge strategy preferred by the field.
@@ -41,9 +43,9 @@ func createDelegatingStrategy(options Options) *delegatingStrategy {
 func (v delegatingStrategy) MergeList(diff apply.ListElement) (apply.Result, error) {
 	// TODO: Support retainkeys
 	switch diff.GetFieldMergeType() {
-	case "merge":
+	case apply.MergeStrategy:
 		return v.merge.MergeList(diff)
-	case "replace":
+	case apply.ReplaceStrategy:
 		return v.replace.MergeList(diff)
 	default:
 		return v.replace.MergeList(diff)
@@ -55,9 +57,9 @@ func (v delegatingStrategy) MergeList(diff apply.ListElement) (apply.Result, err
 func (v delegatingStrategy) MergeMap(diff apply.MapElement) (apply.Result, error) {
 	// TODO: Support retainkeys
 	switch diff.GetFieldMergeType() {
-	case "merge":
+	case apply.MergeStrategy:
 		return v.merge.MergeMap(diff)
-	case "replace":
+	case apply.ReplaceStrategy:
 		return v.replace.MergeMap(diff)
 	default:
 		return v.merge.MergeMap(diff)
@@ -69,9 +71,9 @@ func (v delegatingStrategy) MergeMap(diff apply.MapElement) (apply.Result, error
 func (v delegatingStrategy) MergeType(diff apply.TypeElement) (apply.Result, error) {
 	// TODO: Support retainkeys
 	switch diff.GetFieldMergeType() {
-	case "merge":
+	case apply.MergeStrategy:
 		return v.merge.MergeType(diff)
-	case "replace":
+	case apply.ReplaceStrategy:
 		return v.replace.MergeType(diff)
 	default:
 		return v.merge.MergeType(diff)

pkg/kubectl/apply/visitor.go

@@ -56,8 +56,13 @@ type Result struct {
 	MergedResult interface{}
 }
 
-// MapValuesElement exposes how to get the field / key - value pairs out of a Map or Type Element
-type MapValuesElement interface {
-	Element
-	GetValues() map[string]Element
-}
+const (
+	// MergeStrategy is the strategy to merge the local and remote values
+	MergeStrategy = "merge"
+
+	// RetainKeysStrategy is the strategy to merge the local and remote values, but drop any fields not defined locally
+	RetainKeysStrategy = "retainKeys"
+
+	// ReplaceStrategy is the strategy to replace the remote value with the local value
+	ReplaceStrategy = "replace"
+)