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

Remove duplicate "not found" from some error messages #2047

Merged
merged 1 commit into from Jul 2, 2021

Conversation

thaJeztah
Copy link
Member

I noticed this when building a Dockerfile that failed because a file didn't
exist, so went through error messages that looked like they had a duplicate
"not found" in the output;

[+] Building 0.9s (6/9)
 => [internal] load build definition from Dockerfile                0.2s
 => => transferring dockerfile: 306B                                0.0s
 => [internal] load .dockerignore                                   0.1s
 => => transferring context: 2B                                     0.0s
 => [internal] load metadata for docker.io/library/alpine:latest    0.0s
 => CACHED [1/5] FROM docker.io/library/alpine                      0.0s
 => [internal] load build context                                   0.6s
 => => transferring context: 701B                                   0.5s
 => ERROR [2/5] ADD no-such-file.txt /                              0.0s
------
 > [2/5] ADD no-such-file.txt /:
------
failed to compute cache key: "/no-such-file.txt" not found: not found

I noticed this when building a Dockerfile that failed because a file didn't
exist, so went through error messages that looked like they had a duplicate
"not found" in the output;

    [+] Building 0.9s (6/9)
     => [internal] load build definition from Dockerfile                0.2s
     => => transferring dockerfile: 306B                                0.0s
     => [internal] load .dockerignore                                   0.1s
     => => transferring context: 2B                                     0.0s
     => [internal] load metadata for docker.io/library/alpine:latest    0.0s
     => CACHED [1/5] FROM docker.io/library/alpine                      0.0s
     => [internal] load build context                                   0.6s
     => => transferring context: 701B                                   0.5s
     => ERROR [2/5] ADD no-such-file.txt /                              0.0s
    ------
     > [2/5] ADD no-such-file.txt /:
    ------
    failed to compute cache key: "/no-such-file.txt" not found: not found

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@@ -610,7 +610,7 @@ func (cc *cacheContext) checksum(ctx context.Context, root *iradix.Node, txn *ir
return nil, false, err
}
if cr == nil {
return nil, false, errors.Wrapf(errNotFound, "%q not found", convertKeyToPath(origk))
return nil, false, errors.Wrapf(errNotFound, "%q", convertKeyToPath(origk))
Copy link
Member Author

Choose a reason for hiding this comment

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

I kept the %q formatting here, but perhaps it's not needed, and could be changed to

Suggested change
return nil, false, errors.Wrapf(errNotFound, "%q", convertKeyToPath(origk))
return nil, false, errors.Wrap(errNotFound, convertKeyToPath(origk))

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually; I think the %q could be useful; looking at the original error;

failed to compute cache key: "/no-such-file.txt" not found: not found

Which would now become

failed to compute cache key: "/no-such-file.txt": not found

Removing the quotes would make it;

failed to compute cache key: /no-such-file.txt: not found

Which "works" for simple cases, but possibly in other cases, e.g. empty filename, or path with spaces, would be not so clear.

@tonistiigi
Copy link
Member

I guess a cleaner way is to use a typed error with a property for the path. Not found cases can also use stdlib PathError

@tonistiigi
Copy link
Member

@thaJeztah

@tonistiigi tonistiigi merged commit f5c34a0 into moby:master Jul 2, 2021
@thaJeztah thaJeztah deleted the improve_errors branch July 2, 2021 08:36
@thaJeztah
Copy link
Member Author

I guess a cleaner way is to use a typed error with a property for the path. Not found cases can also use stdlib PathError

Ah, sorry, didn't come round to looking into that; let me know if you still want me to do so

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

2 participants