Merge pull request #5369 from thaJeztah/improve_flag_errors

frontend/dockerfile: BFlags.Parse: include flag with "--" prefix in errors
moby · Oct 1, 2024 · a52c750 · a52c750
2 parents 28d9752 + 31a7c17
commit a52c750
Show file tree

Hide file tree

Showing 3 changed files with 22 additions and 15 deletions.
diff --git a/frontend/dockerfile/dockerfile_mount_test.go b/frontend/dockerfile/dockerfile_mount_test.go
@@ -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?")
 
 	dockerfile = []byte(`

diff --git a/frontend/dockerfile/instructions/bflag.go b/frontend/dockerfile/instructions/bflag.go
@@ -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
@@ -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) {
@@ -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)
 

diff --git a/frontend/dockerfile/instructions/parse_test.go b/frontend/dockerfile/instructions/parse_test.go
@@ -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",