Address review suggestions

Signed-off-by: Rashmi Gottipati <chowdary.grashmi@gmail.com>

pkg/kapp/crdupgradesafety/change_validator.go

@@ -73,9 +73,9 @@ func EnumChangeValidation(diff FieldDiff) (bool, error) {
 
 // RequiredFieldChangeValidation adds a validation check to ensure that
 // existing required fields can be marked as optional in a CRD schema:
-// - No new values are added as required that did not previously have
+// - No new values can be added as required that did not previously have
 // any required fields present
-// - Existing values are removed from the required field
+// - Existing values can be removed from the required field
 // This function returns:
 // - A boolean representation of whether or not the change
 // has been fully handled (i.e. the only change was to required field values)
@@ -88,7 +88,7 @@ func RequiredFieldChangeValidation(diff FieldDiff) (bool, error) {
 	}
 
 	if len(diff.Old.Required) == 0 && len(diff.New.Required) > 0 {
-		return handled(), fmt.Errorf("new values added as required when previously no required fields existed")
+		return handled(), fmt.Errorf("new values added as required when previously no required fields existed: %+v", diff.New.Required)
 	}
 
 	oldSet := sets.NewString()

test/e2e/preflight_crdupgradesafety_invalid_field_change_requiredfield_added_test.go

@@ -14,7 +14,6 @@ func TestPreflightCRDUpgradeSafetyInvalidFieldChangeRequiredFieldAdded(t *testin
 	env := BuildEnv(t)
 	logger := Logger{}
 	kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger}
-	kubectl := Kubectl{t, env.Namespace, logger}
 
 	testName := "preflightcrdupgradesafetyinvalidfieldchangerequiredfieldadded"
 
@@ -68,7 +67,6 @@ spec:
 
 	cleanUp := func() {
 		kapp.Run([]string{"delete", "-a", appName})
-		RemoveClusterResource(t, "ns", testName, "", kubectl)
 	}
 	cleanUp()
 	defer cleanUp()
@@ -126,5 +124,6 @@ spec:
 		_, err = kapp.RunWithOpts([]string{"deploy", "--preflight=CRDUpgradeSafety", "-a", appName, "-f", "-"},
 			RunOpts{StdinReader: strings.NewReader(update), AllowError: true})
 		require.Error(t, err)
+		require.Contains(t, err.Error(), "new required fields added")
 	})
 }

test/e2e/preflight_crdupgradesafety_invalid_field_change_requiredfield_added_when_none_existed_test.go

@@ -14,7 +14,6 @@ func TestPreflightCRDUpgradeSafetyInvalidFieldChangeRequiredFieldAddedWhenNoneEx
 	env := BuildEnv(t)
 	logger := Logger{}
 	kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger}
-	kubectl := Kubectl{t, env.Namespace, logger}
 
 	testName := "preflightcrdupgradesafetyinvalidfieldchangerequiredfieldaddedwhennonexisted"
 
@@ -66,7 +65,6 @@ spec:
 
 	cleanUp := func() {
 		kapp.Run([]string{"delete", "-a", appName})
-		RemoveClusterResource(t, "ns", testName, "", kubectl)
 	}
 	cleanUp()
 	defer cleanUp()
@@ -123,5 +121,6 @@ spec:
 		_, err = kapp.RunWithOpts([]string{"deploy", "--preflight=CRDUpgradeSafety", "-a", appName, "-f", "-"},
 			RunOpts{StdinReader: strings.NewReader(update), AllowError: true})
 		require.Error(t, err)
+		require.Contains(t, err.Error(), "new values added as required when previously no required fields existed")
 	})
 }

test/e2e/preflight_crdupgradesafety_valid_field_change_requiredfield_removed_test.go

@@ -14,7 +14,6 @@ func TestPreflightCRDUpgradeSafetyInvalidFieldChangeRequiredFieldRemoved(t *test
 	env := BuildEnv(t)
 	logger := Logger{}
 	kapp := Kapp{t, env.Namespace, env.KappBinaryPath, logger}
-	kubectl := Kubectl{t, env.Namespace, logger}
 
 	testName := "preflightcrdupgradesafetyinvalidfieldchangerequiredfieldremoved"
 
@@ -69,7 +68,6 @@ spec:
 
 	cleanUp := func() {
 		kapp.Run([]string{"delete", "-a", appName})
-		RemoveClusterResource(t, "ns", testName, "", kubectl)
 	}
 	cleanUp()
 	defer cleanUp()