Merge pull request #96 from lavalamp/fixvalue

disable unions, since they're still experimental

Author: k8s-ci-robot

internal/fixture/state.go

@@ -261,6 +261,8 @@ type TestCase struct {
 	// Managed, if not nil, is the ManagedFields as expected
 	// after all operations are run.
 	Managed fieldpath.ManagedFields
+	// Set to true if the test case needs the union behavior enabled.
+	RequiresUnions bool
 }
 
 // Test runs the test-case using the given parser and a dummy converter.
@@ -283,6 +285,9 @@ func (tc TestCase) BenchWithConverter(parser typed.ParseableType, converter merg
 		Updater: &merge.Updater{Converter: converter},
 		Parser:  parser,
 	}
+	if tc.RequiresUnions {
+		state.Updater.EnableUnionFeature()
+	}
 	// We currently don't have any test that converts, we can take
 	// care of that later.
 	for i, ops := range tc.Ops {
@@ -300,6 +305,17 @@ func (tc TestCase) TestWithConverter(parser typed.ParseableType, converter merge
 		Updater: &merge.Updater{Converter: converter},
 		Parser:  parser,
 	}
+	if tc.RequiresUnions {
+		state.Updater.EnableUnionFeature()
+	} else {
+		// Also test it with unions on.
+		tc2 := tc
+		tc2.RequiresUnions = true
+		err := tc2.TestWithConverter(parser, converter)
+		if err != nil {
+			return fmt.Errorf("fails if unions are on: %v", err)
+		}
+	}
 	// We currently don't have any test that converts, we can take
 	// care of that later.
 	for i, ops := range tc.Ops {

merge/union_test.go

@@ -67,6 +67,7 @@ var unionFieldsParser = func() typed.ParseableType {
 func TestUnion(t *testing.T) {
 	tests := map[string]TestCase{
 		"union_apply_owns_discriminator": {
+			RequiresUnions: true,
 			Ops: []Operation{
 				Apply{
 					Manager:    "default",
@@ -91,6 +92,7 @@ func TestUnion(t *testing.T) {
 			},
 		},
 		"union_apply_without_discriminator_conflict": {
+			RequiresUnions: true,
 			Ops: []Operation{
 				Update{
 					Manager:    "controller",
@@ -125,6 +127,7 @@ func TestUnion(t *testing.T) {
 			},
 		},
 		"union_apply_with_null_value": {
+			RequiresUnions: true,
 			Ops: []Operation{
 				Apply{
 					Manager:    "default",
@@ -138,6 +141,7 @@ func TestUnion(t *testing.T) {
 			},
 		},
 		"union_apply_multiple_unions": {
+			RequiresUnions: true,
 			Ops: []Operation{
 				Apply{
 					Manager:    "default",
@@ -176,6 +180,7 @@ func TestUnion(t *testing.T) {
 func TestUnionErrors(t *testing.T) {
 	tests := map[string]TestCase{
 		"union_apply_two": {
+			RequiresUnions: true,
 			Ops: []Operation{
 				Apply{
 					Manager:    "default",
@@ -188,6 +193,7 @@ func TestUnionErrors(t *testing.T) {
 			},
 		},
 		"union_apply_two_and_discriminator": {
+			RequiresUnions: true,
 			Ops: []Operation{
 				Apply{
 					Manager:    "default",
@@ -201,6 +207,7 @@ func TestUnionErrors(t *testing.T) {
 			},
 		},
 		"union_apply_wrong_discriminator": {
+			RequiresUnions: true,
 			Ops: []Operation{
 				Apply{
 					Manager:    "default",

merge/update.go

@@ -31,6 +31,14 @@ type Converter interface {
 // merge the object on Apply.
 type Updater struct {
 	Converter Converter
+
+	enableUnions bool
+}
+
+// EnableUnionFeature turns on union handling. It is disabled by default until the
+// feature is complete.
+func (s *Updater) EnableUnionFeature() {
+	s.enableUnions = true
 }
 
 func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, workflow string, force bool) (fieldpath.ManagedFields, error) {
@@ -112,9 +120,12 @@ func (s *Updater) update(oldObject, newObject *typed.TypedValue, version fieldpa
 // PATCH call), and liveObject must be the original object (empty if
 // this is a CREATE call).
 func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, manager string) (*typed.TypedValue, fieldpath.ManagedFields, error) {
-	newObject, err := liveObject.NormalizeUnions(newObject)
-	if err != nil {
-		return nil, fieldpath.ManagedFields{}, err
+	var err error
+	if s.enableUnions {
+		newObject, err = liveObject.NormalizeUnions(newObject)
+		if err != nil {
+			return nil, fieldpath.ManagedFields{}, err
+		}
 	}
 	managers = shallowCopyManagers(managers)
 	managers, err = s.update(liveObject, newObject, version, managers, manager, true)
@@ -144,9 +155,12 @@ func (s *Updater) Update(liveObject, newObject *typed.TypedValue, version fieldp
 // and return it.
 func (s *Updater) Apply(liveObject, configObject *typed.TypedValue, version fieldpath.APIVersion, managers fieldpath.ManagedFields, manager string, force bool) (*typed.TypedValue, fieldpath.ManagedFields, error) {
 	managers = shallowCopyManagers(managers)
-	configObject, err := configObject.NormalizeUnionsApply(configObject)
-	if err != nil {
-		return nil, fieldpath.ManagedFields{}, err
+	var err error
+	if s.enableUnions {
+		configObject, err = configObject.NormalizeUnionsApply(configObject)
+		if err != nil {
+			return nil, fieldpath.ManagedFields{}, err
+		}
 	}
 	newObject, err := liveObject.Merge(configObject)
 	if err != nil {

typed/typed.go

@@ -153,6 +153,8 @@ func (tv TypedValue) RemoveItems(items *fieldpath.Set) *TypedValue {
 // - If discriminator changed to non-nil, all other fields but the
 // discriminated one will be cleared,
 // - Otherwise, If only one field is left, update discriminator to that value.
+//
+// Please note: union behavior isn't finalized yet and this is still experimental.
 func (tv TypedValue) NormalizeUnions(new *TypedValue) (*TypedValue, error) {
 	var errs ValidationErrors
 	var normalizeFn = func(w *mergingWalker) {
@@ -177,6 +179,8 @@ func (tv TypedValue) NormalizeUnions(new *TypedValue) (*TypedValue, error) {
 // NormalizeUnionsApply specifically normalize unions on apply. It
 // validates that the applied union is correct (there should be no
 // ambiguity there), and clear the fields according to the sent intent.
+//
+// Please note: union behavior isn't finalized yet and this is still experimental.
 func (tv TypedValue) NormalizeUnionsApply(new *TypedValue) (*TypedValue, error) {
 	var errs ValidationErrors
 	var normalizeFn = func(w *mergingWalker) {