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

Merged
merged 1 commit into from
May 28, 2024

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 😄 )

@JonCrowther JonCrowther merged commit e7fe01a into rancher:release/v2.9 May 28, 2024
2 checks passed
@JonCrowther JonCrowther deleted the replace-multierror branch May 28, 2024 13:37
@brandond
Copy link
Contributor

brandond commented May 28, 2024

I'm curious how we started using multierror, as wrangler has https://github.com/rancher/wrangler/blob/master/pkg/merr/error.go which does similar things if you use merr.Errors and merr.NewErrors() - this package used to be the idiomatic one for use when accumulating errors.

krunalhinguu added a commit to krunalhinguu/rancher that referenced this pull request May 28, 2024
snasovich added a commit that referenced this pull request May 29, 2024
Revert "Replace multierror.Append with errors.Join (#45344)"
JonCrowther added a commit to JonCrowther/rancher that referenced this pull request Jun 4, 2024
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

5 participants