Add rule for arg / stage name comment descriptions

Signed-off-by: Talon Bowler <[email protected]>

frontend/dockerfile/dockerfile_lint_test.go

@@ -46,8 +46,71 @@ 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
+`)
+	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 stage/arg should follow the format: `# <arg/stage name> <description>`. If this is not intended to be a description comment, add an empty line/comment between them.",
+				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 stage/arg should follow the format: `# <arg/stage name> <description>`. If this is not intended to be a description comment, add an empty line/comment between them.",
+				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 stage/arg should follow the format: `# <arg/stage name> <description>`. If this is not intended to be a description comment, add an empty line/comment between them.",
+				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 stage/arg should follow the format: `# <arg/stage name> <description>`. If this is not intended to be a description comment, add an empty line/comment between them.",
+				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
@@ -233,30 +296,35 @@ COPY $bar .
 
 func testRuleCheckOption(t *testing.T, sb integration.Sandbox) {
 	dockerfile := []byte(`#check=skip=all
+#
 FROM scratch as base
 copy Dockerfile .
 `)
 	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
 
 	dockerfile = []byte(`#check=skip=all;error=true
+#
 FROM scratch as base
 copy Dockerfile .
 `)
 	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
 
 	dockerfile = []byte(`#check=skip=ConsistentInstructionCasing,FromAsCasing
+#
 FROM scratch as base
 copy Dockerfile .
 `)
 	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
 
 	dockerfile = []byte(`#check=skip=ConsistentInstructionCasing,FromAsCasing;error=true
+#
 FROM scratch as base
 copy Dockerfile .
 `)
 	checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
 
 	dockerfile = []byte(`#check=skip=ConsistentInstructionCasing
+#
 FROM scratch as base
 copy Dockerfile .
 `)
@@ -268,13 +336,14 @@ copy Dockerfile .
 				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:        2,
+				Line:        3,
 				Level:       1,
 			},
 		},
 	})
 
 	dockerfile = []byte(`#check=skip=ConsistentInstructionCasing;error=true
+#
 FROM scratch as base
 copy Dockerfile .
 `)
@@ -286,7 +355,7 @@ copy Dockerfile .
 				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:        2,
+				Line:        3,
 				Level:       1,
 			},
 		},
@@ -296,6 +365,7 @@ copy Dockerfile .
 	})
 
 	dockerfile = []byte(`#check=skip=all
+#
 FROM scratch as base
 copy Dockerfile .
 `)
@@ -307,7 +377,7 @@ copy Dockerfile .
 				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:        2,
+				Line:        3,
 				Level:       1,
 			},
 		},
@@ -320,6 +390,7 @@ copy Dockerfile .
 	})
 
 	dockerfile = []byte(`#check=error=true
+#
 FROM scratch as base
 copy Dockerfile .
 `)
@@ -334,9 +405,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
@@ -349,22 +422,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
@@ -378,7 +452,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,
 			},
 		},
@@ -414,6 +488,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
 `)
@@ -426,7 +501,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,
 			},
 		},
 	})
@@ -454,6 +529,7 @@ COPY Dockerfile /bar
 
 	dockerfile = []byte(`
 # warning: 'frOM' should be either lowercased or uppercased
+#
 frOM scratch as base
 from scratch as base2
 `)
@@ -465,7 +541,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,
 			},
 		},
@@ -493,6 +569,7 @@ copy Dockerfile /bar
 
 	dockerfile = []byte(`
 # warning: 'from' should match command majority's casing (uppercase)
+#
 from scratch
 COPY Dockerfile /foo
 COPY Dockerfile /bar
@@ -506,14 +583,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
@@ -527,7 +605,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,
 			},
 		},

frontend/dockerfile/docs/rules/_index.md

@@ -100,5 +100,9 @@ $ docker build --check .
       <td><a href="./copy-ignored-file/">CopyIgnoredFile</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 stage/arg should follow the format: `# <arg/stage name> <description>`. If this is not intended to be a description comment, add an empty line/comment between them.</td>
+    </tr>
   </tbody>
 </table>

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

@@ -0,0 +1,50 @@
+---
+title: InvalidDefinitionDescription
+description: Comment for stage/arg should follow the format: `# <arg/stage name> <description>`. If this is not intended to be a description comment, add an empty line/comment between them.
+aliases:
+  - /go/dockerfile/rule/invalid-definition-description/
+---
+
+## Output
+
+```text
+Comment for stage/arg should follow the format: `# <arg/stage name> <description>`. If this is not intended to be a description comment, add an empty line/comment between them.
+```
+
+## Description
+
+When using `docker build` in conjunction with the `--check=outline` or `--check=targets` arguments, `buildx` can parse comments to show descriptions for arguments and stages. This functionality relies on the descriptive comments being particularly formatted. In the cases where comments immediately preceding the `FROM` or `ARG` commands are not meant to be descriptions, add an empty line / comment between them.
+
+## 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 This is the base stage.
+FROM scratch AS base
+# VERSION This is the version number.
+ARG VERSION=1
+```
+

frontend/dockerfile/instructions/parse.go

@@ -100,7 +100,12 @@ 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
+		}
+		validateDefinitionDescription("FROM", []string{fromCmd.Name}, node.PrevComment, node.Location(), lint)
+		return fromCmd, nil
 	case command.Onbuild:
 		return parseOnBuild(req)
 	case command.Workdir:
@@ -122,7 +127,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)
 	}
@@ -857,3 +871,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)
+}