-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
frontend/dockerfile: BFlags.Parse: include flag with "--" prefix in errors #5369
Conversation
@@ -115,7 +115,7 @@ RUN --mont=target=/mytmp,type=tmpfs /bin/true | |||
}, | |||
}, nil) | |||
require.Error(t, err) | |||
require.Contains(t, err.Error(), "unknown flag: mont") | |||
require.Contains(t, err.Error(), "unknown flag: --mont") | |||
require.Contains(t, err.Error(), "did you mean mount?") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably also look at this one to say did you mean --mount?
, but I think the same error-type is used in multiple situations, so it probably needs some changes to make it preserve --
, but that not to affect the "matching";
buildkit/util/suggest/error.go
Lines 58 to 70 in 185751b
type suggestError struct { | |
err error | |
match string | |
} | |
func (e *suggestError) Error() string { | |
return e.err.Error() + " (did you mean " + e.match + "?)" | |
} | |
// Unwrap returns the underlying error. | |
func (e *suggestError) Unwrap() error { | |
return e.err | |
} |
Maybe the --
could be trimmed as part of the Search (not sure), but then it would have to be added again when printing, so this needs a slightly more deeper dive into how this all works;
buildkit/util/suggest/error.go
Line 47 in 185751b
match, ok := Search(val, options, caseSensitive) |
0228644
to
663f98c
Compare
Failure is unrelated; opened a PR to fix it; |
Hm... looks like I broke something here;
|
Minor optimization; put the check for a literal "--" first, instead of matching for "--" as prefix. Also add some documentation to the code, to outline why we return early. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
663f98c
to
429f67f
Compare
|
||
arg, value, hasValue := strings.Cut(arg[2:], "=") | ||
flagName, value, hasValue := strings.Cut(a, "=") | ||
arg := a[2:] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ this line seems to be the bug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DOH!!! I see what I did there; was originally taking a slightly different approach 🙈 . Let me fix that
…rrors Before this change, only the name of the flag would be printed as part of errors. Change the errors to include the flag-name _including_ the "--" prefix to make it more clear the error is about a flag that was provided but invalid. Before this change: Dockerfile:2 -------------------- 1 | FROM alpine 2 | >>> COPY --nosuchflag . . 3 | -------------------- ERROR: failed to solve: dockerfile parse error on line 2: unknown flag: nosuchflag Dockerfile:4 -------------------- 2 | FROM alpine 3 | WORKDIR /test 4 | >>> COPY --exclude /.git . . 5 | -------------------- ERROR: failed to solve: dockerfile parse error on line 4: missing a value on flag: exclude With this change; Dockerfile:2 -------------------- 1 | FROM alpine 2 | >>> COPY --nosuchflag . . 3 | -------------------- ERROR: failed to solve: dockerfile parse error on line 2: unknown flag: --nosuchflag Dockerfile:4 -------------------- 2 | FROM alpine 3 | WORKDIR /test 4 | >>> COPY --exclude /.git . . 5 | -------------------- ERROR: failed to solve: dockerfile parse error on line 4: missing a value on flag: --exclude Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
429f67f
to
31a7c17
Compare
OK; mistake was fixed, and CI is happy now, so this should be ready for review 🤗 |
frontend/dockerfile: BFlags.Parse: return earlier on "--" terminator
Minor optimization; put the check for a literal "--" first, instead of
matching for "--" as prefix. Also add some documentation to the code,
to outline why we return early.
from the POSIX Utility Syntax Guidelines;
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02
frontend/dockerfile: BFlags.Parse: include flag with "--" prefix in errors
Before this change, only the name of the flag would be printed as part
of errors. Change the errors to include the flag-name including the
"--" prefix to make it more clear the error is about a flag that was
provided but invalid.
Before this change:
With this change;