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.

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

Replace multierror.Append with errors.Join #45344

Open
wants to merge 1 commit into
base: release/v2.9
Choose a base branch
from

Conversation

JonCrowther
Copy link
Contributor

Issue:

Problem

The package go-multierror from here uses the MPL 2.0 license, which is considered an incompatible license in rancher.

Solution

Multierrors can be replaced with errors.Join and get similar results. Multierror is formatted a little nicer, but both combine errors and can be checked for the existence of specific errors.

Here is an example of the formatting difference:
https://go.dev/play/p/qYefpBQ81GL

While I was working on removing multierror, I also replaced https://github.com/pkg/errors with the errors standard library. Specifically, I replaced instances of errors.Wrap(err, "message") with fmt.Errorf("message: %w", err). Both wrap the error and create the same error message. The motivation behind this is that https://github.com/pkg/errors has been archived and hasn't received updates since 2020. In a future PR it should be completely removed from rancher.

It's still imported as an indirect requirement, but rancher no longer makes use of it directly.

Testing

The existing unit tests pass. This does not change functionality, just how the errors are handled.

Engineering Testing

Manual Testing

Automated Testing

  • Test types added/modified:
    • Unit
    • Integration (Go Framework)
    • Integration (v2prov Framework)
    • Validation (Go Framework)
    • Other - Explain: EXPLAIN
    • None
    • REMOVE NOT APPLICABLE BULLET POINTS ABOVE
  • If "None" - Reason: EXPLAIN THE REASON
  • If "None" - GH Issue/PR: LINK TO GH ISSUE/PR TO ADD TESTS

Summary: TODO

QA Testing Considerations

Regressions Considerations

TODO

Existing / newly added automated tests that provide evidence there are no regressions:

  • TODO

Copy link
Contributor

@raulcabello raulcabello left a comment

Choose a reason for hiding this comment

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

Code looks good to me. It makes sense to migrate to error.Join
Errors will now be longer as the separator for errors.Join is a new line \n instead of : , but I think that should not be a problem.

Copy link
Contributor

@bigkevmcd bigkevmcd left a comment

Choose a reason for hiding this comment

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

@JonCrowther Thanks for the play example, nice way to show the differences (I have a slight preference for the Go multi error version, I think the go-multierror one is harder to internationalise 😄 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants