From 97bfff2c58c7a1547b48f1140d95707fb79089e5 Mon Sep 17 00:00:00 2001 From: Rashmi Gottipati Date: Tue, 23 Apr 2024 16:45:17 -0400 Subject: [PATCH] Address review suggestions Signed-off-by: Rashmi Gottipati --- pkg/kapp/crdupgradesafety/change_validator.go | 6 +++--- ...esafety_invalid_field_change_requiredfield_added_test.go | 3 +-- ...eld_change_requiredfield_added_when_none_existed_test.go | 3 +-- ...esafety_valid_field_change_requiredfield_removed_test.go | 2 -- 4 files changed, 5 insertions(+), 9 deletions(-) diff --git a/pkg/kapp/crdupgradesafety/change_validator.go b/pkg/kapp/crdupgradesafety/change_validator.go index a475c605a..f346a0549 100644 --- a/pkg/kapp/crdupgradesafety/change_validator.go +++ b/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() diff --git a/test/e2e/preflight_crdupgradesafety_invalid_field_change_requiredfield_added_test.go b/test/e2e/preflight_crdupgradesafety_invalid_field_change_requiredfield_added_test.go index 7c43f30be..125c1f803 100644 --- a/test/e2e/preflight_crdupgradesafety_invalid_field_change_requiredfield_added_test.go +++ b/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") }) } diff --git a/test/e2e/preflight_crdupgradesafety_invalid_field_change_requiredfield_added_when_none_existed_test.go b/test/e2e/preflight_crdupgradesafety_invalid_field_change_requiredfield_added_when_none_existed_test.go index eed64b905..20a0c8126 100644 --- a/test/e2e/preflight_crdupgradesafety_invalid_field_change_requiredfield_added_when_none_existed_test.go +++ b/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") }) } diff --git a/test/e2e/preflight_crdupgradesafety_valid_field_change_requiredfield_removed_test.go b/test/e2e/preflight_crdupgradesafety_valid_field_change_requiredfield_removed_test.go index d7b03e48b..98831efa8 100644 --- a/test/e2e/preflight_crdupgradesafety_valid_field_change_requiredfield_removed_test.go +++ b/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()