Add a temporary drop-in replacement for errors.Join by akerouanton · Pull Request #46188 · moby/moby

akerouanton · 2023-08-10T15:58:30Z

- What I did

As we have a hard time figuring out what #46099 should look like, this drop-in replacement will solve the initial formatting problem we have. It's made internal such that we can remove it whenever we want and unlike #46099 doesn't require thoughtful API changes.

- How I did it

This new joinError has the exact same semantic has the one from stdlib. Only difference is how its Error() method format its message.

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2023-08-10T16:29:20Z

LOL

ERROR: failed to solve: tonistiigi/xx:1.2.1: failed to authorize: failed to fetch oauth token: unexpected status from POST request to https://auth.docker.io/token: 429 Too Many Requests
Error: buildx bake failed with: ERROR: failed to solve: tonistiigi/xx:1.2.1: failed to authorize: failed to fetch oauth token: unexpected status from POST request to https://auth.docker.io/token: 429 Too Many Requests

ERROR: failed to solve: golang:1.20.7-bullseye: unexpected status from HEAD request to https://registry-1.docker.io/v2/library/golang/manifests/1.20.7-bullseye: 503 Service Unavailable
Error: buildx bake failed with: ERROR: failed to solve: golang:1.20.7-bullseye: unexpected status from HEAD request to https://registry-1.docker.io/v2/library/golang/manifests/1.20.7-bullseye: 503 Service Unavailable

thaJeztah

LGTM

akerouanton · 2023-08-10T18:32:50Z

For the record, here's what I get out of the API:

Error response from daemon: 1 errors occurred:
	* invalid endpoint config for network testnet: 5 errors occurred:
		* invalid IPv4 address: foobar
		* invalid IPv6 address: foobar
		* invalid link-local IP address yolo
		* no predefined subnet or ip-range contain the IP address: foobar
		* user specified IP address is supported only when connecting to networks with user configured subnets

thaJeztah · 2023-08-11T08:16:36Z

One thing I'm wondering now, looking at the output; it looks like we have quite some indentation; Perhaps we should remove 1 level;

Error response from daemon: 1 errors occurred:
* invalid endpoint config for network testnet: 5 errors occurred:
	* invalid IPv4 address: foobar
	* invalid IPv6 address: foobar
	* invalid link-local IP address yolo
	* no predefined subnet or ip-range contain the IP address: foobar
	* user specified IP address is supported only when connecting to networks with user configured subnets

And we should omit the N errors occurred if there's only one error at the top-level.

Some variations:

A. Just remove the top-level counter

:question_mark: Does it feel weird if there's nothing at the first line of the error that summarizes "whats' wrong"?

Error response from daemon:
* invalid endpoint config for network testnet: 5 errors occurred:
	* invalid IPv4 address: foobar
	* invalid IPv6 address: foobar
	* invalid link-local IP address yolo
	* no predefined subnet or ip-range contain the IP address: foobar
	* user specified IP address is supported only when connecting to networks with user configured subnets

B. Remove the counter and use the first "wrapper" error as summary

If there's only one error, we can use that error as the summary;

Error response from daemon: invalid endpoint config for network testnet: 5 errors occurred:
	* invalid IPv4 address: foobar
	* invalid IPv6 address: foobar
	* invalid link-local IP address yolo
	* no predefined subnet or ip-range contain the IP address: foobar
	* user specified IP address is supported only when connecting to networks with user configured subnets

C. Do we need the counter... at all?

However, now the question is: does the N errors occurred actually add much value?

MAYBE it's useful for the "outer" error, because it provides a summary of the error at the first line.
But with all nested errors also showing, it almost feels as if the count is "wrong"; in the below example, should it show 8 errors occurred ?
Do the numbers add much value to end-user, or is it just "noise"?
We should be careful using errors occurred. Using "error occurred" is easy to interpret as "something went wrong" (on "our side", i.e. 500 internal server error)
...which can easily lead to users opening bug reports "bug: failing to run container: 5 errors occurred:"

Error response from daemon: 2 errors occurred:
* invalid endpoint config for network testnet: 5 errors occurred:
	* invalid IPv4 address: foobar
	* invalid IPv6 address: foobar
	* invalid link-local IP address yolo
	* no predefined subnet or ip-range contain the IP address: foobar
	* user specified IP address is supported only when connecting to networks with user configured subnets
* invalid endpoint config for network foobar: 3 errors occurred:
	* invalid IPv4 address: foobar
	* invalid IPv6 address: foobar
	* invalid link-local IP address yolo

If we leave out the secondary counts, at least the top level counter is somewhat less ambiguous;

Error response from daemon: 2 errors occurred:
* invalid endpoint config for network testnet:
	* invalid IPv4 address: foobar
	* invalid IPv6 address: foobar
	* invalid link-local IP address yolo
	* no predefined subnet or ip-range contain the IP address: foobar
	* user specified IP address is supported only when connecting to networks with user configured subnets
* invalid endpoint config for network foobar:
	* invalid IPv4 address: foobar
	* invalid IPv6 address: foobar
	* invalid link-local IP address yolo

And ... a that point, maybe (?) the whole count is not needed;

Error response from daemon:
* invalid endpoint config for network testnet:
	* invalid IPv4 address: foobar
	* invalid IPv6 address: foobar
	* invalid link-local IP address yolo
	* no predefined subnet or ip-range contain the IP address: foobar
	* user specified IP address is supported only when connecting to networks with user configured subnets
* invalid endpoint config for network foobar:
	* invalid IPv4 address: foobar
	* invalid IPv6 address: foobar
	* invalid link-local IP address yolo

thaJeztah · 2023-08-11T08:16:57Z

(Off-topic) should we always have a summary on the first line?

So... not sure what's best for this. As mentioned earlier it may feel a bit odd for the first line to only show Error response from daemon:

In the examples on this PR, errors are due to networking-config validation, so we could add a summary;

Error response from daemon: invalid networking-config provided:
* invalid endpoint config for network testnet:
	* invalid IPv4 address: foobar
	* invalid IPv6 address: foobar
	* invalid link-local IP address yolo
	* no predefined subnet or ip-range contain the IP address: foobar
	* user specified IP address is supported only when connecting to networks with user configured subnets
* invalid endpoint config for network foobar:
	* invalid IPv4 address: foobar
	* invalid IPv6 address: foobar
	* invalid link-local IP address yolo

Looking what it would look like if there's (validation) errors in other areas, e.g.;

Error response from daemon:
* invalid networking-config provided:
	* invalid endpoint config for network testnet:
		* invalid IPv4 address: foobar
		* invalid IPv6 address: foobar
		* invalid link-local IP address yolo
		* no predefined subnet or ip-range contain the IP address: foobar
		* user specified IP address is supported only when connecting to networks with user configured subnets
	* invalid endpoint config for network foobar:
		* invalid IPv4 address: foobar
		* invalid IPv6 address: foobar
		* invalid link-local IP address yolo
* invalid mount-config provided:
	* invalid host-path for bind-mount "/src":
		* host-path must be absolute: "../../src/node_modules/"
	* invalid config for volume "database-data":
		* invalid mode: "RW": must be one of 'rw', 'ro'

However... now we're back at square 1: the first line doesn't have a summary. We can still add a "summary" at the top, but maybe that's just ... noise;

Error response from daemon: invalid configuration provided:
* invalid networking-config provided:
	* invalid endpoint config for network testnet:
		* invalid IPv4 address: foobar
		* invalid IPv6 address: foobar
		* invalid link-local IP address yolo
		* no predefined subnet or ip-range contain the IP address: foobar
		* user specified IP address is supported only when connecting to networks with user configured subnets
	* invalid endpoint config for network foobar:
		* invalid IPv4 address: foobar
		* invalid IPv6 address: foobar
		* invalid link-local IP address yolo
* invalid mount-config provided:
	* invalid host-path for bind-mount "/src":
		* host-path must be absolute: "../../src/node_modules/"
	* invalid config for volume "database-data":
		* invalid mode: "RW": must be one of 'rw', 'ro'

internal/multierror/multierror.go

akerouanton · 2023-08-16T08:07:43Z

And ... a that point, maybe (?) the whole count is not needed;

Going back to this PR, and even before I reached this line I also thought the count doesn't add any value. I'll remove it.

Except this, all other formatting suggestions aren't tied to this specific PR, but to #46183. So let's discuss that there.

akerouanton · 2023-08-16T09:12:36Z

internal/multierror/multierror_test.go

+)
+
+func TestErrorJoin(t *testing.T) {
+	err := Join(errors.New("foobar"), fmt.Errorf("invalid config:\n%w", Join(errors.New("foo"), errors.New("bar"))))


If we use this instead:

err := Join(errors.New("foobar"), Join(errors.New("foo"), errors.New("bar")))

The output will be clumsy:

* foobar * * foo * bar

This is not great, but I believe it's better to defer to Join callers to do the right thing instead (eg. add an error count, a nice preamble message, etc...).

The newline is a bit awkward indeed. Just gave it a quick try to see if we can make that automatic, but... it's tricky.

thaJeztah

Left some suggestions; also pushed a quick branch if any of those make sense to you; akerouanton#4

internal/multierror/multierror.go

internal/multierror/multierror_test.go

internal/multierror/multierror.go

As we have a hard time figuring out what moby#46099 should look like, this drop-in replacement will solve the initial formatting problem we have. It's made internal such that we can remove it whenever we want and unlike moby#46099 doesn't require thoughtful API changes. Signed-off-by: Albin Kerouanton <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]>

thaJeztah

LGTM

akerouanton mentioned this pull request Aug 10, 2023

api: Endpoints can now return a tree of errors #46099

Open

thaJeztah marked this pull request as ready for review August 10, 2023 18:10

thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Aug 10, 2023

thaJeztah added this to the 25.0.0 milestone Aug 10, 2023

thaJeztah approved these changes Aug 10, 2023

View reviewed changes

neersighted reviewed Aug 11, 2023

View reviewed changes

internal/multierror/multierror.go Show resolved Hide resolved

akerouanton mentioned this pull request Aug 10, 2023

daemon: Improve NetworkingConfig & EndpointSettings validation #46183

Merged

akerouanton force-pushed the custom-multierror branch 2 times, most recently from 0b5b756 to d2c4700 Compare August 16, 2023 09:06

akerouanton commented Aug 16, 2023

View reviewed changes

akerouanton force-pushed the custom-multierror branch from d2c4700 to fe0e08e Compare August 16, 2023 09:20

thaJeztah mentioned this pull request Aug 16, 2023

multierror: take single errors into account akerouanton/docker#4

Closed

thaJeztah reviewed Aug 16, 2023

View reviewed changes

internal/multierror/multierror.go Outdated Show resolved Hide resolved

internal/multierror/multierror_test.go Outdated Show resolved Hide resolved

internal/multierror/multierror.go Show resolved Hide resolved

akerouanton force-pushed the custom-multierror branch from fe0e08e to 64de635 Compare August 16, 2023 14:19

thaJeztah approved these changes Aug 16, 2023

View reviewed changes

thaJeztah merged commit d83ead8 into moby:master Aug 16, 2023

akerouanton deleted the custom-multierror branch August 16, 2023 15:18

akerouanton mentioned this pull request Aug 19, 2023

multierror: make it consistent when there's a single error #46275

Open

thaJeztah mentioned this pull request Aug 29, 2023

daemon/containerd: Replace pkg/errors with std errors #46353

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a temporary drop-in replacement for errors.Join#46188

Add a temporary drop-in replacement for errors.Join#46188
thaJeztah merged 1 commit intomoby:masterfrom
akerouanton:custom-multierror

akerouanton commented Aug 10, 2023

Uh oh!

thaJeztah commented Aug 10, 2023 •

edited

Loading

Uh oh!

thaJeztah left a comment

Uh oh!

akerouanton commented Aug 10, 2023

Uh oh!

thaJeztah commented Aug 11, 2023

Uh oh!

thaJeztah commented Aug 11, 2023

Uh oh!

Uh oh!

akerouanton commented Aug 16, 2023

Uh oh!

akerouanton Aug 16, 2023

Uh oh!

thaJeztah Aug 16, 2023

Uh oh!

thaJeztah left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

akerouanton commented Aug 10, 2023

Uh oh!

thaJeztah commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

akerouanton commented Aug 10, 2023

Uh oh!

thaJeztah commented Aug 11, 2023

A. Just remove the top-level counter

B. Remove the counter and use the first "wrapper" error as summary

C. Do we need the counter... at all?

Uh oh!

thaJeztah commented Aug 11, 2023

(Off-topic) should we always have a summary on the first line?

Uh oh!

Uh oh!

akerouanton commented Aug 16, 2023

Uh oh!

akerouanton Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Aug 16, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

thaJeztah commented Aug 10, 2023 •

edited

Loading