Ensure fixed order in multi-line error returned by change validator #1047
Conversation
…hange validator Signed-off-by: Artur Zych <[email protected]>
azych
force-pushed
the
ensure-order-in-changevalidator-combinederror
branch
from
d5d9267 to
4870ff1
Compare
There was a problem hiding this comment.
It looks good to me!
Do you mind letting it stay up for today? Just in case @everettraven has any thoughts around it? We can work on merging it away if he is not.
The changes seem reasonable, but I will loop him in because he had contributed this pieces 🙌🏼
hey @100mik, I definitely don't mind and thank you taking a look at it! |
| @@ -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)) { | |||
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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:
ChangeValidator.Validate()doeserrors.Join()and returns combined errorvalidator.Validate()doesfmt.Errorf()and wraps this returned error with additional contextvalidator.Validate()doeserrors.join()on the previous error (together with potential other errors from different validations) and returns the combined result to the user
|
hey @100mik just a friendly nudge to see if you had some time to take another look at this and if it can be moved forward potentially? Thanks! |
What this PR does / why we need it:
This PR introduces order to potentially multi-line error returned by ChangeValidator Validate(). Currently field validations within Validate() happen based on order returned by a map, which is not fixed/deterministic and means that the same set of internal errors can produce a different "unique" error each time, because Validate() combines all errors into a single one before it returns.
This can cause issues on the users side - eg. with errors returned by Validate ending up in objects .Status.Conditions which in turn could mean that a controller could be forced to try and reconcile status/object based on the same (yet different) error message. Please see a real-life example here: operator-framework/operator-controller#1456
Which issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?
Additional Notes for your reviewer:
Review Checklist:
a link to that PR
change
Additional documentation e.g., Proposal, usage docs, etc.: