diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 4c46ad7cfb768..590ff3401cbee 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 @@ -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,7 +422,7 @@ 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, }, { @@ -357,7 +430,7 @@ FROM scratch AS base3 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, }, }, @@ -365,6 +438,7 @@ FROM scratch AS base3 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,7 +583,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 (uppercase)", - Line: 3, + Line: 4, Level: 1, }, }, @@ -514,6 +591,7 @@ COPY Dockerfile /baz 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, }, }, 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) + }, + } )