Merge pull request #5208 from daghack/invalid-definition-description-…

…check

InvalidDefinitionDescription Rule Check

frontend/dockerfile/dockerfile_lint_test.go

@@ -46,8 +46,79 @@ var lintTests = integration.TestFuncs(
 	testInvalidDefaultArgInFrom,
 	testFromPlatformFlagConstDisallowed,
 	testCopyIgnoredFiles,
+	testDefinitionDescription,
 )
 
+func testDefinitionDescription(t *testing.T, sb integration.Sandbox) {
+	dockerfile := []byte(`
+# foo this is the foo
+ARG foo=bar
+
+# base this is the base image
+FROM scratch AS base
+
+# version this is the version number
+ARG version=latest
+
+# baz this is the baz
+ARG foo=baz bar=qux baz=quux
+#
+ARG bit=bat
+
+# comment for something other than ARG or FROM
+COPY Dockerfile .
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
+
+	dockerfile = []byte(`
+# bar this is the bar
+ARG foo=bar
+# BasE this is the BasE image
+FROM scratch AS base
+# definitely a bad comment
+ARG version=latest
+# definitely a bad comment
+ARG foo=baz bar=qux baz=quux
+`)
+	checkLinterWarnings(t, sb, &lintTestParams{
+		Dockerfile: dockerfile,
+		Warnings: []expectedLintWarning{
+			{
+				RuleName:    "InvalidDefinitionDescription",
+				Description: "Comment for build stage or argument should follow the format: `# <arg/stage name> <description>`. If this is not intended to be a description comment, add an empty line or comment between the instruction and the comment.",
+				URL:         "https://docs.docker.com/go/dockerfile/rule/invalid-definition-description/",
+				Detail:      "Comment for ARG should follow the format: `# foo <description>`",
+				Level:       1,
+				Line:        3,
+			},
+			{
+				RuleName:    "InvalidDefinitionDescription",
+				Description: "Comment for build stage or argument should follow the format: `# <arg/stage name> <description>`. If this is not intended to be a description comment, add an empty line or comment between the instruction and the comment.",
+				URL:         "https://docs.docker.com/go/dockerfile/rule/invalid-definition-description/",
+				Detail:      "Comment for FROM should follow the format: `# base <description>`",
+				Level:       1,
+				Line:        5,
+			},
+			{
+				RuleName:    "InvalidDefinitionDescription",
+				Description: "Comment for build stage or argument should follow the format: `# <arg/stage name> <description>`. If this is not intended to be a description comment, add an empty line or comment between the instruction and the comment.",
+				URL:         "https://docs.docker.com/go/dockerfile/rule/invalid-definition-description/",
+				Detail:      "Comment for ARG should follow the format: `# version <description>`",
+				Level:       1,
+				Line:        7,
+			},
+			{
+				RuleName:    "InvalidDefinitionDescription",
+				Description: "Comment for build stage or argument should follow the format: `# <arg/stage name> <description>`. If this is not intended to be a description comment, add an empty line or comment between the instruction and the comment.",
+				URL:         "https://docs.docker.com/go/dockerfile/rule/invalid-definition-description/",
+				Detail:      "Comment for ARG should follow the format: `# <arg_key> <description>`",
+				Level:       1,
+				Line:        9,
+			},
+		},
+	})
+}
+
 func testCopyIgnoredFiles(t *testing.T, sb integration.Sandbox) {
 	dockerignore := []byte(`
 Dockerfile
@@ -368,9 +439,11 @@ copy Dockerfile .
 func testStageName(t *testing.T, sb integration.Sandbox) {
 	dockerfile := []byte(`
 # warning: stage name should be lowercase
+#
 FROM scratch AS BadStageName
 
 # warning: 'as' should match 'FROM' cmd casing.
+#
 FROM scratch as base2
 
 FROM scratch AS base3
@@ -383,22 +456,23 @@ FROM scratch AS base3
 				Description: "Stage names should be lowercase",
 				URL:         "https://docs.docker.com/go/dockerfile/rule/stage-name-casing/",
 				Detail:      "Stage name 'BadStageName' should be lowercase",
-				Line:        3,
+				Line:        4,
 				Level:       1,
 			},
 			{
 				RuleName:    "FromAsCasing",
 				Description: "The 'as' keyword should match the case of the 'from' keyword",
 				URL:         "https://docs.docker.com/go/dockerfile/rule/from-as-casing/",
 				Detail:      "'as' and 'FROM' keywords' casing do not match",
-				Line:        6,
+				Line:        8,
 				Level:       1,
 			},
 		},
 	})
 
 	dockerfile = []byte(`
 # warning: 'AS' should match 'from' cmd casing.
+#
 from scratch AS base
 
 from scratch as base2
@@ -412,7 +486,7 @@ from scratch as base2
 				Description: "The 'as' keyword should match the case of the 'from' keyword",
 				URL:         "https://docs.docker.com/go/dockerfile/rule/from-as-casing/",
 				Detail:      "'AS' and 'from' keywords' casing do not match",
-				Line:        3,
+				Line:        4,
 				Level:       1,
 			},
 		},
@@ -448,6 +522,7 @@ COPY Dockerfile \
 func testConsistentInstructionCasing(t *testing.T, sb integration.Sandbox) {
 	dockerfile := []byte(`
 # warning: 'FROM' should be either lowercased or uppercased
+#
 From scratch as base
 FROM scratch AS base2
 `)
@@ -460,7 +535,7 @@ FROM scratch AS base2
 				URL:         "https://docs.docker.com/go/dockerfile/rule/consistent-instruction-casing/",
 				Detail:      "Command 'From' should match the case of the command majority (uppercase)",
 				Level:       1,
-				Line:        3,
+				Line:        4,
 			},
 		},
 	})
@@ -488,6 +563,7 @@ COPY Dockerfile /bar
 
 	dockerfile = []byte(`
 # warning: 'frOM' should be either lowercased or uppercased
+#
 frOM scratch as base
 from scratch as base2
 `)
@@ -499,7 +575,7 @@ from scratch as base2
 				Description: "All commands within the Dockerfile should use the same casing (either upper or lower)",
 				URL:         "https://docs.docker.com/go/dockerfile/rule/consistent-instruction-casing/",
 				Detail:      "Command 'frOM' should match the case of the command majority (lowercase)",
-				Line:        3,
+				Line:        4,
 				Level:       1,
 			},
 		},
@@ -527,6 +603,7 @@ copy Dockerfile /bar
 
 	dockerfile = []byte(`
 # warning: 'from' should match command majority's casing (uppercase)
+#
 from scratch
 COPY Dockerfile /foo
 COPY Dockerfile /bar
@@ -540,14 +617,15 @@ COPY Dockerfile /baz
 				Description: "All commands within the Dockerfile should use the same casing (either upper or lower)",
 				URL:         "https://docs.docker.com/go/dockerfile/rule/consistent-instruction-casing/",
 				Detail:      "Command 'from' should match the case of the command majority (uppercase)",
-				Line:        3,
+				Line:        4,
 				Level:       1,
 			},
 		},
 	})
 
 	dockerfile = []byte(`
 # warning: 'FROM' should match command majority's casing (lowercase)
+#
 FROM scratch
 copy Dockerfile /foo
 copy Dockerfile /bar
@@ -561,7 +639,7 @@ copy Dockerfile /baz
 				Description: "All commands within the Dockerfile should use the same casing (either upper or lower)",
 				URL:         "https://docs.docker.com/go/dockerfile/rule/consistent-instruction-casing/",
 				Detail:      "Command 'FROM' should match the case of the command majority (lowercase)",
-				Line:        3,
+				Line:        4,
 				Level:       1,
 			},
 		},
@@ -1382,6 +1460,12 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestPara
 			require.Less(t, lintResults.Error.Location.SourceIndex, int32(len(lintResults.Sources)))
 		}
 
+		if len(warnings) != len(lintResults.Warnings) {
+			for _, w := range lintResults.Warnings {
+				t.Logf("Warning Received: %s\n", w.Detail)
+			}
+		}
+
 		require.Equal(t, len(warnings), len(lintResults.Warnings))
 
 		sort.Slice(lintResults.Warnings, func(i, j int) bool {

frontend/dockerfile/docs/rules/_index.md

@@ -103,5 +103,9 @@ To learn more about how to use build checks, see
       <td><a href="./copy-ignored-file/">CopyIgnoredFile (experimental)</a></td>
       <td>Attempting to Copy file that is excluded by .dockerignore</td>
     </tr>
+    <tr>
+      <td><a href="./invalid-definition-description/">InvalidDefinitionDescription</a></td>
+      <td>Comment for build stage or argument should follow the format: `# <arg/stage name> <description>`. If this is not intended to be a description comment, add an empty line or comment between the instruction and the comment.</td>
+    </tr>
   </tbody>
 </table>

frontend/dockerfile/docs/rules/invalid-definition-description.md

@@ -0,0 +1,66 @@
+---
+title: InvalidDefinitionDescription
+description: Comment for build stage or argument should follow the format: `# <arg/stage name> <description>`. If this is not intended to be a description comment, add an empty line or comment between the instruction and the comment.
+aliases:
+  - /go/dockerfile/rule/invalid-definition-description/
+---
+
+## Output
+
+```text
+Comment for build stage or argument should follow the format: `# <arg/stage name> <description>`. If this is not intended to be a description comment, add an empty line or comment between the instruction and the comment.
+```
+
+## Description
+
+The [`--call=outline`](https://docs.docker.com/reference/cli/docker/buildx/build/#call-outline)
+and [`--call=targets`](https://docs.docker.com/reference/cli/docker/buildx/build/#call-outline)
+flags for the `docker build` command print descriptions for build targets and arguments.
+The descriptions are generated from [Dockerfile comments](https://docs.docker.com/reference/cli/docker/buildx/build/#descriptions)
+that immediately precede the `FROM` or `ARG` instruction
+and that begin with the name of the build stage or argument.
+For example:
+
+```dockerfile
+# build-cli builds the CLI binary
+FROM alpine AS build-cli
+# VERSION controls the version of the program
+ARG VERSION=1
+```
+
+In cases where preceding comments are not meant to be descriptions,
+add an empty line or comment between the instruction and the preceding comment.
+
+## Examples
+
+❌ Bad: A non-descriptive comment on the line preceding the `FROM` command.
+
+```dockerfile
+# a non-descriptive comment
+FROM scratch AS base
+
+# another non-descriptive comment
+ARG VERSION=1
+```
+
+✅ Good: An empty line separating non-descriptive comments.
+
+```dockerfile
+# a non-descriptive comment
+
+FROM scratch AS base
+
+# another non-descriptive comment
+
+ARG VERSION=1
+```
+
+✅ Good: Comments describing `ARG` keys and stages immediately proceeding the command.
+
+```dockerfile
+# base is a stage for compiling source
+FROM scratch AS base
+# VERSION This is the version number.
+ARG VERSION=1
+```
+

frontend/dockerfile/instructions/parse.go

@@ -100,7 +100,14 @@ func ParseInstructionWithLinter(node *parser.Node, lint *linter.Linter) (v inter
 			msg := linter.RuleFromAsCasing.Format(req.command, req.args[1])
 			lint.Run(&linter.RuleFromAsCasing, node.Location(), msg)
 		}
-		return parseFrom(req)
+		fromCmd, err := parseFrom(req)
+		if err != nil {
+			return nil, err
+		}
+		if fromCmd.Name != "" {
+			validateDefinitionDescription("FROM", []string{fromCmd.Name}, node.PrevComment, node.Location(), lint)
+		}
+		return fromCmd, nil
 	case command.Onbuild:
 		return parseOnBuild(req)
 	case command.Workdir:
@@ -122,7 +129,16 @@ func ParseInstructionWithLinter(node *parser.Node, lint *linter.Linter) (v inter
 	case command.StopSignal:
 		return parseStopSignal(req)
 	case command.Arg:
-		return parseArg(req)
+		argCmd, err := parseArg(req)
+		if err != nil {
+			return nil, err
+		}
+		argKeys := []string{}
+		for _, arg := range argCmd.Args {
+			argKeys = append(argKeys, arg.Key)
+		}
+		validateDefinitionDescription("ARG", argKeys, node.PrevComment, node.Location(), lint)
+		return argCmd, nil
 	case command.Shell:
 		return parseShell(req)
 	}
@@ -858,3 +874,22 @@ func doesFromCaseMatchAsCase(req parseRequest) bool {
 	}
 	return req.args[1] == strings.ToUpper(req.args[1])
 }
+
+func validateDefinitionDescription(instruction string, argKeys []string, descComments []string, location []parser.Range, lint *linter.Linter) {
+	if len(descComments) == 0 || len(argKeys) == 0 {
+		return
+	}
+	descCommentParts := strings.Split(descComments[len(descComments)-1], " ")
+	for _, key := range argKeys {
+		if key == descCommentParts[0] {
+			return
+		}
+	}
+	exampleKey := argKeys[0]
+	if len(argKeys) > 1 {
+		exampleKey = "<arg_key>"
+	}
+
+	msg := linter.RuleInvalidDefinitionDescription.Format(instruction, exampleKey)
+	lint.Run(&linter.RuleInvalidDefinitionDescription, location, msg)
+}