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

refactor(orc8r): Omit nil-checks for multierror.Append calls #12666

Merged
merged 1 commit into from
May 11, 2022

Conversation

wolfseb
Copy link
Contributor

@wolfseb wolfseb commented May 10, 2022

Summary

multierror.Append already handles nil-checks, so checking before calling it is unnecessary, except when the error is formatted with e.g. fmt.Errorf. This PR follows this discussion.

Test Plan

Run unit tests

Additional Information

  • This change is backwards-breaking

@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines. label May 10, 2022
@github-actions
Copy link
Contributor

Thanks for opening a PR! 💯

A couple initial guidelines

Howto

  • Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
  • Checks. All required CI checks must pass before merge.
  • Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

If this is your first Magma PR, also consider reading

@github-actions github-actions bot added the component: orc8r Orchestrator-related issue label May 10, 2022
Signed-off-by: Sebastian Wolf <sebastian.wolf@tngtech.com>
@github-actions
Copy link
Contributor

github-actions bot commented May 10, 2022

feg-workflow

    2 files  202 suites   37s ⏱️
371 tests 371 ✔️ 0 💤 0
385 runs  385 ✔️ 0 💤 0

Results for commit c23375c.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

cloud-workflow

    7 files  356 suites   3m 0s ⏱️
979 tests 979 ✔️ 0 💤 0

Results for commit c23375c.

@github-actions
Copy link
Contributor

agw-workflow

     77 files     122 suites   6m 55s ⏱️
1 159 tests 1 150 ✔️ 9 💤 0
1 160 runs  1 151 ✔️ 9 💤 0

Results for commit c23375c.

@wolfseb wolfseb marked this pull request as ready for review May 11, 2022 07:39
@wolfseb wolfseb requested review from a team and uri200 May 11, 2022 07:39
if err != nil {
errs = multierror.Append(errs, errors.Wrapf(err, "collect garbage for table %+v", tableName))
}
errs = multierror.Append(errs, errors.Wrapf(err, "collect garbage for table %+v", tableName))
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this nil check might make things more complicated if we intend to get rid of errors.Wrapf later? We will have to reintroduce it then, I suppose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you are right. Since it is merged now we'll have to change it in the PR that gets rid of Wrapf (#12682); I will arrange that.

@uri200 uri200 merged commit 5d092bb into magma:master May 11, 2022
@wolfseb wolfseb deleted the cleanup-multierror branch May 11, 2022 22:30
@wolfseb wolfseb restored the cleanup-multierror branch May 11, 2022 22:31
@wolfseb wolfseb deleted the cleanup-multierror branch May 11, 2022 23:43
emakeev pushed a commit to emakeev/magma that referenced this pull request Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: orc8r Orchestrator-related issue size/M Denotes a PR that changes 30-99 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants