refactor(orc8r): Replace multi.go with hashicorp/go-multierror module

[alexzurbonsen]

Summary

This PR modifies Magma's multi error management to use the hashicorp/go-multierror module.

Formatting of multierrors changes due to the format used in the hashicorp module.
The new output looks like this:

"x errors occured:
	* error
	* error
	* error
	... \n\n"

whereas the former output was

"errors: [index: error; index: error; ...]"

where error is a string that contains the error message, and in many cases additional parts with further details abouth the error.

hashicorp/go-multierror is already used in this repo, often in conjunction with the Errorf function from github.com/pkg/errors. Since it is not longer maintained we use the standard lib fmt.Errorf in this PR. We also opened an issue to remove the pkg/errors from magma #12632.

Since now unused, we delete multi.go and respective tests. Plus grpc_to_http.go, which was already unused before.

Test Plan

• Precommit checks in orc8r/cloud and feg/gateway./build.py -c
• Build orc8r ./build.py --all

Additional Information

Done in pairing with @mpfirrmann.

[github-actions]

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]

feg-workflow

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

Results for commit f2aa99a.

♻️ This comment has been updated with latest results.

[github-actions]

cloud-workflow

    7 files  356 suites   2m 3s ⏱️
985 tests 985 ✔️ 0 💤 0 ❌

Results for commit f2aa99a.

♻️ This comment has been updated with latest results.

[github-actions]

agw-workflow

     77 files     122 suites   7m 21s ⏱️
1 162 tests 1 153 ✔️ 9 💤 0 ❌
1 163 runs  1 154 ✔️ 9 💤 0 ❌

Results for commit b0dcf3a.

♻️ This comment has been updated with latest results.

[sebathomas]

The insync-check is failing, you probably need some more go mod clean-up.

[maxhbr]

The pr title prefix is fairly generic and does not match best practices, maybe better would be refactor(orc8r): .

[maxhbr]

LGTM

[uri200]

Why are we removing our own implementation of errors if we already have it?
Why aren't we removing hashicorp instead to avoid that module not being supported in the future? (same that happened with errors module)

Note we also try to maintain the errors in one single line. So printing them in multiple lines is something we should avoid. Otherwise debugging and parsing becomes difficult. (for example you can not use grep properly anymore and you need to look for previous lines to see the full error)

[uri200]

i know this has been mentioned below, but just as a placeholder: remove all ifs since Append will ignore nils

[maxhbr]

this contains formatting, the errors. Errorf result will still not be nil, even if err was nil.

But this 3 line block could be extracted to a function, since they just differ in a few words.

[alexzurbonsen]

Introduced a wrapper in this file.

[uri200]

why are we introducing an unsupported package here

#12632

[maxhbr]

The ticket #12632 was created as a result of realizing that the unsupported package was used here. I think this is for consistence but I agree that this PR should maybe also fix (parts of) #12632.

[alexzurbonsen]

@uri200 The package is removed in the final commit of this PR, so this is not part of the changes. (The PR isn't nicely tailored for commit by commit review, sorry.)

[emakeev]

The ticket #12632 was created as a result of realizing that the unsupported package was used here. I think this is for consistence but I agree that this PR should maybe also fix (parts of) #12632.

Could you please elaborate? We introduce 12627 which we are not sure, we need and then we create #12632 to fix issues that #12627 will create?

[maxhbr]

Oh, sorry, that was not formulated clear enough.

#12632 was created while working on #12627. But it will also be solved here for the touched code. So no new instances of #12632 will be introduced and the code commented here is actually fixed in a later commit, as @alexzurbonsen explained.

[alexzurbonsen]

Cleaned up the commit history. Now all changes that replace merrors by hasicorp/go-multierror are in one commit.

[maxhbr]

Why aren't we removing hashicorp instead to avoid that module not being supported in the future? (same that happened with errors module)

I expect that the chances that this package is better maintained than our own library function are pretty high. It is also well tested and due to its wide usage it is also probably already known to many contributors.

Note we also try to maintain the errors in one single line.

That is a good point!

[uri200]

Let's wait for @emakeev , he may have some context on multi error code.

[emakeev]

Could you please explain what is the purpose of this change?
The usage pattern seems to be nearly identical,
the implementation already exists & in use and hasn't been problematic so far.

Is there any reason, we want to spend time of developers & reviewers to fix something which is not broken while there are so many real improvements Magma can benefit from?

If you just need to change the format of the errors' string (which also has to be justified, BTW), it could be accomplished by a small 4 lines PR vs this 400 lines PR and keep flexibility to modify the format later anytime you need/want, just change func (me *Multi) Error() string to:

func (me *Multi) Error() string {
	if me == nil {
		return "<nil>"
	}
	switch len(me.errors) {
	case 0:
		return ""
	case 1:
		return me.errors[0].Error()
	default:
		var b bytes.Buffer
		fmt.Fprintf(&b, "%d errors occurred:\n", len(me.errors))
		fmtStr := "\t* %v\n"
		for _, e := range me.errors {
			fmt.Fprintf(&b, fmtStr, e)
		}
		b.Write([]byte("\n"))
		return b.String()
	}
}

--- a/orc8r/lib/go/merrors/multi.go
+++ b/orc8r/lib/go/merrors/multi.go
@@ -41,12 +41,12 @@ func (me *Multi) Error() string {
                return me.errors[0].Error()
        default:
                var b bytes.Buffer
-               fmtStr := "errors: [%d: %v"
-               for i, e := range me.errors {
-                       fmt.Fprintf(&b, fmtStr, i, e)
-                       fmtStr = "; %d: %v"
+               fmt.Fprintf(&b, "%d errors occurred:\n", len(me.errors))
+               fmtStr := "\t* %v\n"
+               for _, e := range me.errors {
+                       fmt.Fprintf(&b, fmtStr, e)
                }
-               b.Write([]byte("]"))
+               b.Write([]byte("\n"))
                return b.String()
        }
 }

or something like this...

[emakeev]

I expect that the chances that this package is better maintained than our own library function are pretty high. It is also well tested and due to its wide usage it is also probably already known to many contributors.

note that it also applies to the rest of Magma codebase (merror is maintained as well as the rest of Magma code), the original implementation is simple, controlled by us, allows us to format multi errors ANY way we want or need, now and in the future and it relies only on std Go packages.
But - the bigger question is - why fix something which is not broken? What's the incentive to do it?

[maxhbr]

Hey @emakeev,

Thank you for your comments.

There are right now two implementations of multierrors used in Magma:

• our own implementation, used in 5 files and
• the hashicorp one, used in 12 files.

We saw an opportunity to clean up the source code and decided to go for the more commonly used one. The hashicorp library is also widely known, better documented and might rely less on tribal knowledge. Migrating to that also preserves the possibility to add arbitrary ErrorFormat formatters and drops ~300 lines of source code we needed to maintain.

Regarding the formatting change:
Since more often than not, the code already uses the default hashicorp formatting with multiline error messages, we think that this would make it at least more consistent.

[emakeev]

Hey @emakeev,

Thank you for your comments.

There are right now two implementations of multierrors used in Magma:

• our own implementation, used in 5 files and
• the hashicorp one, used in 12 files.

We saw an opportunity to clean up the source code and decided to go for the more commonly used one. The hashicorp library is also widely known, better documented and might rely less on tribal knowledge. Migrating to that also preserves the possibility to add arbitrary ErrorFormat formatters and drops ~300 lines of source code we needed to maintain.

Regarding the formatting change: Since more often than not, the code already uses the default hashicorp formatting with multiline error messages, we think that this would make it at least more consistent.

Sure, it seems to be reasonable to settle on a single implementation & since the work is already done, I don't see an issue w it. I'll leave it to @uri200 to decide if the change to multi line format is acceptable for FeG.

[uri200]

you need to fix the insych issue
Maybe you just need to run ./build.py -g