Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure fixed order in multi-line error returned by change validator #1047

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
+23 −7
Open

Ensure fixed order in multi-line error returned by change validator #1047

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion pkg/kapp/crdupgradesafety/change_validator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I defer any decision making to @100mik as I'm not sure what versioning semantics are used for kapp and it's associated libraries, but if breaking changes to the function signature are allowed it might be worth considering returning the slice of errors for callers to do with what they please.

I hadn't thought about the deterministic error message when I contributed this code, but adding additional time complexity for sorting feels unnecessary if we just provide the list of errors instead of joining them together on behalf of the caller.

Also a side note, the errors.Join() function would return an error that contains a method signature Unwrap() []error that will return the slice of errors included in it. Technically, a caller could get the slice of errors returned by this function today and do their own sorting but it isn't very user friendly.

All that being said, if @100mik is fine with this change as-is I don't have a strong reason against it. Just proposing a potential alternative :)

Copy link
Author

@azych azych Jan 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both good points and I will be happy to adjust this PR according to what maintainers think best.
My idea here was to avoid introducing a breaking change and have a predictable / non-volatile error by default for the end user.

Update: just to help visualize the user effort require to parse this with Unwrap() - there are 3 levels of internal nesting here, going from the bottom of the call stack:

  1. ChangeValidator.Validate() does errors.Join() and returns combined error
  2. validator.Validate() does fmt.Errorf() and wraps this returned error with additional context
  3. validator.Validate() does errors.join() on the previous error (together with potential other errors from different validations) and returns the combined result to the user

diff := diffs[field]

handled := false
for _, validation := range cv.Validations {
ok, err := validation(diff)
Expand Down
23 changes: 17 additions & 6 deletions pkg/kapp/crdupgradesafety/change_validator_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package crdupgradesafety_test

import (
"errors"
"fmt"
"testing"

"carvel.dev/kapp/pkg/kapp/crdupgradesafety"
Expand Down Expand Up @@ -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
100mik marked this conversation as resolved.
Show resolved Hide resolved
}{
{
name: "no changes, no error",
Expand Down Expand Up @@ -347,7 +351,7 @@ func TestChangeValidator(t *testing.T) {
},
},
},
shouldError: true,
expectedError: validationErr1,
},
{
name: "changes, validation failed, change fully handled, error",
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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)
}
})
}
}
Expand Down
Loading