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.

Sign up for GitHub

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

Jump to bottom

frontend/dockerfile: BFlags.Parse: include flag with "--" prefix in errors #5369

Merged
merged 2 commits into from
Oct 1, 2024
Merged

frontend/dockerfile: BFlags.Parse: include flag with "--" prefix in errors #5369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b300989
frontend/dockerfile: BFlags.Parse: return earlier on "--" terminator
thaJeztah Sep 30, 2024
31a7c17
frontend/dockerfile: BFlags.Parse: include flag with "--" prefix in e…
thaJeztah Sep 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion frontend/dockerfile/dockerfile_mount_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?")
Copy link
Member Author

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)


dockerfile = []byte(`
Expand Down
33 changes: 20 additions & 13 deletions frontend/dockerfile/instructions/bflag.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,25 +147,32 @@ func (bf *BFlags) Parse() error {
return errors.Wrap(bf.Err, "error setting up flags")
}

for _, arg := range bf.Args {
if !strings.HasPrefix(arg, "--") {
return errors.Errorf("arg should start with -- : %s", arg)
}

if arg == "--" {
for _, a := range bf.Args {
if a == "--" {
// Stop processing further arguments as flags. We're matching
// the POSIX Utility Syntax Guidelines here;
// https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02
//
// > The first -- argument that is not an option-argument should be accepted
// > as a delimiter indicating the end of options. Any following arguments
// > should be treated as operands, even if they begin with the '-' character.
return nil
}
if !strings.HasPrefix(a, "--") {
return errors.Errorf("arg should start with -- : %s", a)
}

arg, value, hasValue := strings.Cut(arg[2:], "=")
flagName, value, hasValue := strings.Cut(a, "=")
arg := flagName[2:]

flag, ok := bf.flags[arg]
if !ok {
err := errors.Errorf("unknown flag: %s", arg)
err := errors.Errorf("unknown flag: %s", flagName)
return suggest.WrapError(err, arg, allFlags(bf.flags), true)
}

if _, ok = bf.used[arg]; ok && flag.flagType != stringsType {
return errors.Errorf("duplicate flag specified: %s", arg)
return errors.Errorf("duplicate flag specified: %s", flagName)
}

bf.used[arg] = flag
Expand All @@ -174,7 +181,7 @@ func (bf *BFlags) Parse() error {
case boolType:
// value == "" is only ok if no "=" was specified
if hasValue && value == "" {
return errors.Errorf("missing a value on flag: %s", arg)
return errors.Errorf("missing a value on flag: %s", flagName)
}

switch strings.ToLower(value) {
Expand All @@ -183,18 +190,18 @@ func (bf *BFlags) Parse() error {
case "false":
flag.Value = "false"
default:
return errors.Errorf("expecting boolean value for flag %s, not: %s", arg, value)
return errors.Errorf("expecting boolean value for flag %s, not: %s", flagName, value)
}

case stringType:
if !hasValue {
return errors.Errorf("missing a value on flag: %s", arg)
return errors.Errorf("missing a value on flag: %s", flagName)
}
flag.Value = value

case stringsType:
if !hasValue {
return errors.Errorf("missing a value on flag: %s", arg)
return errors.Errorf("missing a value on flag: %s", flagName)
}
flag.StringValues = append(flag.StringValues, value)

Expand Down
2 changes: 1 addition & 1 deletion frontend/dockerfile/instructions/parse_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,7 @@ func TestErrorCases(t *testing.T) {
{
name: "MAINTAINER unknown flag",
dockerfile: "MAINTAINER --boo joe@example.com",
expectedError: "unknown flag: boo",
expectedError: "unknown flag: --boo",
},
{
name: "Chaining ONBUILD",
Expand Down
Loading