diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 4c46ad7cfb768..825febdc36b82 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/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: `# `. 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 `", + Level: 1, + Line: 3, + }, + { + RuleName: "InvalidDefinitionDescription", + Description: "Comment for stage/arg should follow the format: `# `. 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 `", + Level: 1, + Line: 5, + }, + { + RuleName: "InvalidDefinitionDescription", + Description: "Comment for stage/arg should follow the format: `# `. 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 `", + Level: 1, + Line: 7, + }, + { + RuleName: "InvalidDefinitionDescription", + Description: "Comment for stage/arg should follow the format: `# `. 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: `# `", + Level: 1, + Line: 9, + }, + }, + }) +} + func testCopyIgnoredFiles(t *testing.T, sb integration.Sandbox) { dockerignore := []byte(` Dockerfile diff --git a/frontend/dockerfile/docs/rules/_index.md b/frontend/dockerfile/docs/rules/_index.md index 0508c34de9b0d..ffe872993ca38 100644 --- a/frontend/dockerfile/docs/rules/_index.md +++ b/frontend/dockerfile/docs/rules/_index.md @@ -100,5 +100,9 @@ $ docker build --check . CopyIgnoredFile Attempting to Copy file that is excluded by .dockerignore + + InvalidDefinitionDescription + Comment for stage/arg should follow the format: `# `. If this is not intended to be a description comment, add an empty line/comment between them. + diff --git a/frontend/dockerfile/docs/rules/invalid-definition-description.md b/frontend/dockerfile/docs/rules/invalid-definition-description.md new file mode 100644 index 0000000000000..546d6e1410841 --- /dev/null +++ b/frontend/dockerfile/docs/rules/invalid-definition-description.md @@ -0,0 +1,50 @@ +--- +title: InvalidDefinitionDescription +description: Comment for stage/arg should follow the format: `# `. 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: `# `. 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 +``` + diff --git a/frontend/dockerfile/instructions/parse.go b/frontend/dockerfile/instructions/parse.go index 3996c1d0a259b..191a22ef070d7 100644 --- a/frontend/dockerfile/instructions/parse.go +++ b/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 = "" + } + + msg := linter.RuleInvalidDefinitionDescription.Format(instruction, exampleKey) + lint.Run(&linter.RuleInvalidDefinitionDescription, location, msg) +} diff --git a/frontend/dockerfile/linter/docs/InvalidDefinitionDescription.md b/frontend/dockerfile/linter/docs/InvalidDefinitionDescription.md new file mode 100644 index 0000000000000..ce8ad8555e2de --- /dev/null +++ b/frontend/dockerfile/linter/docs/InvalidDefinitionDescription.md @@ -0,0 +1,42 @@ +## Output + +```text +Comment for stage/arg should follow the format: `# `. 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 +``` diff --git a/frontend/dockerfile/linter/ruleset.go b/frontend/dockerfile/linter/ruleset.go index d94c32c53ce7f..ddb060c552104 100644 --- a/frontend/dockerfile/linter/ruleset.go +++ b/frontend/dockerfile/linter/ruleset.go @@ -164,4 +164,12 @@ var ( return fmt.Sprintf("Attempting to %s file %q that is excluded by .dockerignore", cmd, file) }, } + RuleInvalidDefinitionDescription = LinterRule[func(string, string) string]{ + Name: "InvalidDefinitionDescription", + Description: "Comment for stage/arg should follow the format: `# `. 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/", + Format: func(instruction, defName string) string { + return fmt.Sprintf("Comment for %s should follow the format: `# %s `", instruction, defName) + }, + } )