diff --git a/frontend/dockerfile/dockerfile_lint_test.go b/frontend/dockerfile/dockerfile_lint_test.go index 4c46ad7cfb76..be1187c148d1 100644 --- a/frontend/dockerfile/dockerfile_lint_test.go +++ b/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: `# `. 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 `", + Level: 1, + Line: 3, + }, + { + RuleName: "InvalidDefinitionDescription", + Description: "Comment for build stage or argument should follow the format: `# `. 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 `", + Level: 1, + Line: 5, + }, + { + RuleName: "InvalidDefinitionDescription", + Description: "Comment for build stage or argument should follow the format: `# `. 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 `", + Level: 1, + Line: 7, + }, + { + RuleName: "InvalidDefinitionDescription", + Description: "Comment for build stage or argument should follow the format: `# `. 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: `# `", + Level: 1, + Line: 9, + }, + }, + }) +} + func testCopyIgnoredFiles(t *testing.T, sb integration.Sandbox) { dockerignore := []byte(` Dockerfile @@ -233,30 +304,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 +344,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 +363,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 +373,7 @@ copy Dockerfile . }) dockerfile = []byte(`#check=skip=all +# FROM scratch as base copy Dockerfile . `) @@ -307,7 +385,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 +398,7 @@ copy Dockerfile . }) dockerfile = []byte(`#check=error=true +# FROM scratch as base copy Dockerfile . `) @@ -334,9 +413,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 +430,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 +438,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 +446,7 @@ FROM scratch AS base3 dockerfile = []byte(` # warning: 'AS' should match 'from' cmd casing. +# from scratch AS base from scratch as base2 @@ -378,7 +460,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 +496,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 +509,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 +537,7 @@ COPY Dockerfile /bar dockerfile = []byte(` # warning: 'frOM' should be either lowercased or uppercased +# frOM scratch as base from scratch as base2 `) @@ -465,7 +549,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 +577,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 +591,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 +599,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 +613,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, }, }, @@ -1282,6 +1368,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 { diff --git a/frontend/dockerfile/docs/rules/_index.md b/frontend/dockerfile/docs/rules/_index.md index 0508c34de9b0..a892adf50a1f 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 build stage or argument should follow the format: `# `. If this is not intended to be a description comment, add an empty line or comment between the instruction and the comment. + 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 000000000000..3e9044c417f0 --- /dev/null +++ b/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: `# `. 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: `# `. 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 +``` + diff --git a/frontend/dockerfile/instructions/parse.go b/frontend/dockerfile/instructions/parse.go index 3996c1d0a259..7c7a653bfe7c 100644 --- a/frontend/dockerfile/instructions/parse.go +++ b/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) } @@ -857,3 +873,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 000000000000..6f75fb2785ee --- /dev/null +++ b/frontend/dockerfile/linter/docs/InvalidDefinitionDescription.md @@ -0,0 +1,58 @@ +## Output + +```text +Comment for build stage or argument should follow the format: `# `. 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 +``` diff --git a/frontend/dockerfile/linter/ruleset.go b/frontend/dockerfile/linter/ruleset.go index d94c32c53ce7..fd98ade4aaa1 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 build stage or argument should follow the format: `# `. 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/", + Format: func(instruction, defName string) string { + return fmt.Sprintf("Comment for %s should follow the format: `# %s `", instruction, defName) + }, + } )