diff --git a/pkg/kapp/crdupgradesafety/change_validator.go b/pkg/kapp/crdupgradesafety/change_validator.go index f69e67410..a10c572db 100644 --- a/pkg/kapp/crdupgradesafety/change_validator.go +++ b/pkg/kapp/crdupgradesafety/change_validator.go @@ -7,7 +7,9 @@ import ( "bytes" "errors" "fmt" + "maps" "reflect" + "slices" "github.com/openshift/crd-schema-checker/pkg/manifestcomparators" v1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" @@ -449,7 +451,10 @@ func (cv *ChangeValidator) Validate(old, new v1.CustomResourceDefinition) error continue } - for field, diff := range diffs { + // ensure order of the potentially multi-line final error + for _, field := range slices.Sorted(maps.Keys(diffs)) { + diff := diffs[field] + handled := false for _, validation := range cv.Validations { ok, err := validation(diff) diff --git a/pkg/kapp/crdupgradesafety/change_validator_test.go b/pkg/kapp/crdupgradesafety/change_validator_test.go index 2a73261aa..fc999d295 100644 --- a/pkg/kapp/crdupgradesafety/change_validator_test.go +++ b/pkg/kapp/crdupgradesafety/change_validator_test.go @@ -5,6 +5,7 @@ package crdupgradesafety_test import ( "errors" + "fmt" "testing" "carvel.dev/kapp/pkg/kapp/crdupgradesafety" @@ -235,12 +236,15 @@ func TestFlattenSchema(t *testing.T) { } func TestChangeValidator(t *testing.T) { + validationErr1 := errors.New(`version "v1alpha1", field "^" has unknown change, refusing to determine that change is safe`) + validationErr2 := errors.New(`version "v1alpha1", field "^": fail`) + for _, tc := range []struct { name string changeValidator *crdupgradesafety.ChangeValidator old v1.CustomResourceDefinition new v1.CustomResourceDefinition - shouldError bool + expectedError error }{ { name: "no changes, no error", @@ -347,7 +351,7 @@ func TestChangeValidator(t *testing.T) { }, }, }, - shouldError: true, + expectedError: validationErr1, }, { name: "changes, validation failed, change fully handled, error", @@ -384,15 +388,18 @@ func TestChangeValidator(t *testing.T) { }, }, }, - shouldError: true, + expectedError: validationErr2, }, { - name: "changes, validation failed, change not fully handled, error", + name: "changes, validation failed, change not fully handled, ordered error", changeValidator: &crdupgradesafety.ChangeValidator{ Validations: []crdupgradesafety.ChangeValidation{ func(_ crdupgradesafety.FieldDiff) (bool, error) { return false, errors.New("fail") }, + func(_ crdupgradesafety.FieldDiff) (bool, error) { + return false, errors.New("error") + }, }, }, old: v1.CustomResourceDefinition{ @@ -421,12 +428,16 @@ func TestChangeValidator(t *testing.T) { }, }, }, - shouldError: true, + expectedError: fmt.Errorf("%w\n%s\n%w", validationErr2, `version "v1alpha1", field "^": error`, validationErr1), }, } { t.Run(tc.name, func(t *testing.T) { err := tc.changeValidator.Validate(tc.old, tc.new) - assert.Equal(t, tc.shouldError, err != nil, "should error? - %v", tc.shouldError) + if tc.expectedError != nil { + assert.EqualError(t, err, tc.expectedError.Error()) + } else { + assert.NoError(t, err) + } }) } }