refactor(cwg): replace deprecated errors module #12725

MoritzThomasHuebner · 2022-05-16T06:39:54Z

Fixes #12632 for description

Summary

Remove dependency on "github.com/pkg/errors"

Replace use of errors.Wrap[f] with fmt.Errorf
Replace use of pkg errors.New with standard lib errors.New
Update go.mod files
Replace use of errors.New(fmt.Sprintf(...)) with fmt.Errorf
The error handling in addToStore in ue_store_utils.go and in getUE in uesim.go was incorrect.
- In these database transactions commitErr and rollbackErr were not logged.
- We restructured the switch/case logic to if/else
- In the else bracket (err != nil), we now use a multierror to propagate both err and rollbackErr. The logic here is that the rollback should only be triggered if an error happened before, so both errors should be logged.

Test Plan

Run unit tests

Additional Information

Worked in pairing with @wolfseb

This change is backwards-breaking

github-actions · 2022-05-16T06:40:12Z

Thanks for opening a PR! 💯

A couple initial guidelines

All commits must be signed off. This is enforced by DCO check.
All PR titles must follow the semantic commits format. This is enforced by Semantic PR.

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

Contributing Conventions for norms around contributed code

If this is your first Magma PR, also consider reading

Developer Onboarding for onboarding as a new Magma developer
Development Workflow for guidance on your first pull request
CI Checks for points of contact for failing or flaky CI checks
GitHub-to-Slack mappings for Magma maintainers for guidance on how to contact maintainers on Slack

github-actions · 2022-05-16T07:40:54Z

cloud-workflow

979 tests 979 ✔️ 2m 32s ⏱️
355 suites     0 💤
    7 files     0 ❌

Results for commit b10f7a9.

♻️ This comment has been updated with latest results.

alexzurbonsen · 2022-05-17T18:33:49Z

cwf/cloud/go/services/cwf/obsidian/models/validate.go

@@ -45,7 +44,7 @@ func (m *GatewayCwfConfigs) ValidateModel(context.Context) error {
 	for _, peer := range m.AllowedGrePeers {
 		for _, key := range set[peer.IP] {
 			if swag.Uint32Value(peer.Key) == key {
-				return errors.New(fmt.Sprintf("Found duplicate peer %s with key %d", peer.IP, key))
+				return fmt.Errorf("Found duplicate peer %s with key %d", peer.IP, key)


Noticed that the PR description says this would be substituted with errors.New from std lib. I think this way might be nicer (you don't need the Sprintf). But may be adapt the PR summary?

I agree. Also, I stumbled across a few cases like this in #12724, where there was a Sprintf inside an errors.New. When we just change to errors.New from the std lib, the linter will complain and suggest using fmt.Errorf instead, as it is done here.
But yes, we should add this case to the summary.

I updated the summary.

alexzurbonsen · 2022-05-17T18:43:47Z

cwf/gateway/services/uesim/servicers/ue_store_utils.go

+				errs := multierror.Append(err, fmt.Errorf("Error while rolling back transaction: %w", rollbackErr))
+				err = ConvertStorageErrorToGrpcStatus(errs.ErrorOrNil())


Is there a reason to use multierror in one case but not in the other? And for using multierror at all?

And why do we start passing commitErr and rollbackErr?

There is a similar case in cwf/gateway/services/uesim/servicers/uesim.go.

If err is nil, but the commit fails, there is only one error to be passed on. If err is not nil, then it tries to roll back, but if that also fails, we have two errors to pass: err and rollbackErr, that's why we use a multierror here.

We need to pass commitErr , because err is nil by default here due to the if clause, otherwise we would find an error, but the error we pass is nil, so in the end the error gets ignored.
In the other case we use the multierror to pass on both, err and rollbackErr.

I agree with @wolfseb

Ah, so this goes beyond replacing pkg/errors and fixes broken error handling. Probably should mention that in the summary as well.

I added this to the summary.

cwf/gateway/services/uesim/servicers/uesim.go

alexzurbonsen · 2022-06-14T13:25:16Z

@uri200 Want to bring this PR to your attention. Would be great to get a review. Thanks!

Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>

Also fixes an incorrect use of commit/rollback error handling in ue_store_utils.go and uesim.go. Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>

* refactor(cwf): Replaces errors.Wrap[f] with fmt.Errorf. Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com> * refactor(cwf): Replaces more pkg/errors usages with std lib. Also fixes an incorrect use of commit/rollback error handling in ue_store_utils.go and uesim.go. Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>

MoritzThomasHuebner requested review from a team and uri200 May 16, 2022 06:39

pull-request-size bot added the size/L Denotes a Pull Request that changes 100-499 lines. label May 16, 2022

github-actions bot added the component: cwf label May 16, 2022

MoritzThomasHuebner marked this pull request as draft May 16, 2022 06:47

maxhbr mentioned this pull request May 16, 2022

Check whether github.com/pkg/errors can be replaced with standard library functions #12632

Closed

6 tasks

MoritzThomasHuebner changed the title ~~refactor(cwf): Replaces errors.Wrap[f] with fmt.Errorf.~~ refactor(cwg): Replaces errors.Wrap[f] with fmt.Errorf. May 16, 2022

MoritzThomasHuebner force-pushed the Remove_pkg/err_from_cwf branch from f1e9818 to 3bab076 Compare May 16, 2022 07:12

MoritzThomasHuebner force-pushed the Remove_pkg/err_from_cwf branch 2 times, most recently from 5fc09d4 to 9d9e837 Compare May 16, 2022 09:01

MoritzThomasHuebner marked this pull request as ready for review May 16, 2022 23:59

MoritzThomasHuebner changed the title ~~refactor(cwg): Replaces errors.Wrap[f] with fmt.Errorf.~~ refactor(cwg): replace deprecated errors module May 17, 2022

alexzurbonsen reviewed May 17, 2022

View reviewed changes

MoritzThomasHuebner added ready2merge This PR is ready to be merged (is approved and passes all required checks) and removed ready2merge This PR is ready to be merged (is approved and passes all required checks) labels May 18, 2022

MoritzThomasHuebner force-pushed the Remove_pkg/err_from_cwf branch 3 times, most recently from 8cecfd4 to 85592dc Compare May 24, 2022 02:25

magma deleted a comment from github-actions bot May 24, 2022

MoritzThomasHuebner force-pushed the Remove_pkg/err_from_cwf branch 4 times, most recently from 1ff0966 to d0e2cb2 Compare May 31, 2022 04:59

Neudrino reviewed May 31, 2022

View reviewed changes

cwf/gateway/services/uesim/servicers/uesim.go Show resolved Hide resolved

Neudrino approved these changes Jun 1, 2022

View reviewed changes

MoritzThomasHuebner force-pushed the Remove_pkg/err_from_cwf branch from d0e2cb2 to f85393d Compare June 2, 2022 00:34

alexzurbonsen approved these changes Jun 2, 2022

View reviewed changes

MoritzThomasHuebner force-pushed the Remove_pkg/err_from_cwf branch from f85393d to 2b8a0ad Compare June 8, 2022 01:55

MoritzThomasHuebner force-pushed the Remove_pkg/err_from_cwf branch from 2b8a0ad to fbc559a Compare June 9, 2022 02:00

uri200 approved these changes Jun 14, 2022

View reviewed changes

MoritzThomasHuebner added the ready2merge This PR is ready to be merged (is approved and passes all required checks) label Jun 15, 2022

MoritzThomasHuebner added 2 commits June 15, 2022 11:33

refactor(cwf): Replaces errors.Wrap[f] with fmt.Errorf.

ca11795

Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>

refactor(cwf): Replaces more pkg/errors usages with std lib.

b10f7a9

Also fixes an incorrect use of commit/rollback error handling in ue_store_utils.go and uesim.go. Signed-off-by: Moritz Huebner <moritz.huebner@tngtech.com>

MoritzThomasHuebner force-pushed the Remove_pkg/err_from_cwf branch from fbc559a to b10f7a9 Compare June 15, 2022 01:33

MoritzThomasHuebner mentioned this pull request Jun 15, 2022

refactor(orc8r): Move cloud/go/obsidian to cloud/go/services #12742

Merged

1 task

sebathomas merged commit 6992af1 into magma:master Jun 15, 2022

MoritzThomasHuebner deleted the Remove_pkg/err_from_cwf branch July 18, 2022 07:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(cwg): replace deprecated errors module #12725

refactor(cwg): replace deprecated errors module #12725

MoritzThomasHuebner commented May 16, 2022 •

edited

github-actions bot commented May 16, 2022

github-actions bot commented May 16, 2022 •

edited

alexzurbonsen May 17, 2022 •

edited

wolfseb May 17, 2022

MoritzThomasHuebner May 18, 2022

alexzurbonsen May 17, 2022 •

edited

wolfseb May 18, 2022 •

edited

MoritzThomasHuebner May 18, 2022

alexzurbonsen May 18, 2022

MoritzThomasHuebner May 19, 2022

alexzurbonsen commented Jun 14, 2022

		errs := multierror.Append(err, fmt.Errorf("Error while rolling back transaction: %w", rollbackErr))
		err = ConvertStorageErrorToGrpcStatus(errs.ErrorOrNil())

refactor(cwg): replace deprecated errors module #12725

refactor(cwg): replace deprecated errors module #12725

Conversation

MoritzThomasHuebner commented May 16, 2022 • edited

Summary

Test Plan

Additional Information

github-actions bot commented May 16, 2022

Howto

More info

github-actions bot commented May 16, 2022 • edited

cloud-workflow

alexzurbonsen May 17, 2022 • edited

Choose a reason for hiding this comment

wolfseb May 17, 2022

Choose a reason for hiding this comment

MoritzThomasHuebner May 18, 2022

Choose a reason for hiding this comment

alexzurbonsen May 17, 2022 • edited

Choose a reason for hiding this comment

wolfseb May 18, 2022 • edited

Choose a reason for hiding this comment

MoritzThomasHuebner May 18, 2022

Choose a reason for hiding this comment

alexzurbonsen May 18, 2022

Choose a reason for hiding this comment

MoritzThomasHuebner May 19, 2022

Choose a reason for hiding this comment

alexzurbonsen commented Jun 14, 2022

MoritzThomasHuebner commented May 16, 2022 •

edited

github-actions bot commented May 16, 2022 •

edited

alexzurbonsen May 17, 2022 •

edited

alexzurbonsen May 17, 2022 •

edited

wolfseb May 18, 2022 •

edited