Skip to content

Commit 7c4f6d9

Browse files
tonistiigitonistiigi
authored
Merge pull request moby#5208 from daghack/invalid-definition-description-check
InvalidDefinitionDescription Rule Check
2 parents 0bb688c + 6fefbd4 commit 7c4f6d9

File tree

7 files changed

+281
-18
lines changed

7 files changed

+281
-18
lines changed

‎frontend/dockerfile/dockerfile_lint_test.go

Lines changed: 91 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,79 @@ var lintTests = integration.TestFuncs(
4646
testInvalidDefaultArgInFrom,
4747
testFromPlatformFlagConstDisallowed,
4848
testCopyIgnoredFiles,
49+
testDefinitionDescription,
4950
)
5051

52+
func testDefinitionDescription(t *testing.T, sb integration.Sandbox) {
53+
dockerfile := []byte(`
54+
# foo this is the foo
55+
ARG foo=bar
56+
57+
# base this is the base image
58+
FROM scratch AS base
59+
60+
# version this is the version number
61+
ARG version=latest
62+
63+
# baz this is the baz
64+
ARG foo=baz bar=qux baz=quux
65+
#
66+
ARG bit=bat
67+
68+
# comment for something other than ARG or FROM
69+
COPY Dockerfile .
70+
`)
71+
checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})
72+
73+
dockerfile = []byte(`
74+
# bar this is the bar
75+
ARG foo=bar
76+
# BasE this is the BasE image
77+
FROM scratch AS base
78+
# definitely a bad comment
79+
ARG version=latest
80+
# definitely a bad comment
81+
ARG foo=baz bar=qux baz=quux
82+
`)
83+
checkLinterWarnings(t, sb, &lintTestParams{
84+
Dockerfile: dockerfile,
85+
Warnings: []expectedLintWarning{
86+
{
87+
RuleName: "InvalidDefinitionDescription",
88+
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.",
89+
URL: "https://docs.docker.com/go/dockerfile/rule/invalid-definition-description/",
90+
Detail: "Comment for ARG should follow the format: `# foo <description>`",
91+
Level: 1,
92+
Line: 3,
93+
},
94+
{
95+
RuleName: "InvalidDefinitionDescription",
96+
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.",
97+
URL: "https://docs.docker.com/go/dockerfile/rule/invalid-definition-description/",
98+
Detail: "Comment for FROM should follow the format: `# base <description>`",
99+
Level: 1,
100+
Line: 5,
101+
},
102+
{
103+
RuleName: "InvalidDefinitionDescription",
104+
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.",
105+
URL: "https://docs.docker.com/go/dockerfile/rule/invalid-definition-description/",
106+
Detail: "Comment for ARG should follow the format: `# version <description>`",
107+
Level: 1,
108+
Line: 7,
109+
},
110+
{
111+
RuleName: "InvalidDefinitionDescription",
112+
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.",
113+
URL: "https://docs.docker.com/go/dockerfile/rule/invalid-definition-description/",
114+
Detail: "Comment for ARG should follow the format: `# <arg_key> <description>`",
115+
Level: 1,
116+
Line: 9,
117+
},
118+
},
119+
})
120+
}
121+
51122
func testCopyIgnoredFiles(t *testing.T, sb integration.Sandbox) {
52123
dockerignore := []byte(`
53124
Dockerfile
@@ -368,9 +439,11 @@ copy Dockerfile .
368439
func testStageName(t *testing.T, sb integration.Sandbox) {
369440
dockerfile := []byte(`
370441
# warning: stage name should be lowercase
442+
#
371443
FROM scratch AS BadStageName
372444
373445
# warning: 'as' should match 'FROM' cmd casing.
446+
#
374447
FROM scratch as base2
375448
376449
FROM scratch AS base3
@@ -383,22 +456,23 @@ FROM scratch AS base3
383456
Description: "Stage names should be lowercase",
384457
URL: "https://docs.docker.com/go/dockerfile/rule/stage-name-casing/",
385458
Detail: "Stage name 'BadStageName' should be lowercase",
386-
Line: 3,
459+
Line: 4,
387460
Level: 1,
388461
},
389462
{
390463
RuleName: "FromAsCasing",
391464
Description: "The 'as' keyword should match the case of the 'from' keyword",
392465
URL: "https://docs.docker.com/go/dockerfile/rule/from-as-casing/",
393466
Detail: "'as' and 'FROM' keywords' casing do not match",
394-
Line: 6,
467+
Line: 8,
395468
Level: 1,
396469
},
397470
},
398471
})
399472

400473
dockerfile = []byte(`
401474
# warning: 'AS' should match 'from' cmd casing.
475+
#
402476
from scratch AS base
403477
404478
from scratch as base2
@@ -412,7 +486,7 @@ from scratch as base2
412486
Description: "The 'as' keyword should match the case of the 'from' keyword",
413487
URL: "https://docs.docker.com/go/dockerfile/rule/from-as-casing/",
414488
Detail: "'AS' and 'from' keywords' casing do not match",
415-
Line: 3,
489+
Line: 4,
416490
Level: 1,
417491
},
418492
},
@@ -448,6 +522,7 @@ COPY Dockerfile \
448522
func testConsistentInstructionCasing(t *testing.T, sb integration.Sandbox) {
449523
dockerfile := []byte(`
450524
# warning: 'FROM' should be either lowercased or uppercased
525+
#
451526
From scratch as base
452527
FROM scratch AS base2
453528
`)
@@ -460,7 +535,7 @@ FROM scratch AS base2
460535
URL: "https://docs.docker.com/go/dockerfile/rule/consistent-instruction-casing/",
461536
Detail: "Command 'From' should match the case of the command majority (uppercase)",
462537
Level: 1,
463-
Line: 3,
538+
Line: 4,
464539
},
465540
},
466541
})
@@ -488,6 +563,7 @@ COPY Dockerfile /bar
488563

489564
dockerfile = []byte(`
490565
# warning: 'frOM' should be either lowercased or uppercased
566+
#
491567
frOM scratch as base
492568
from scratch as base2
493569
`)
@@ -499,7 +575,7 @@ from scratch as base2
499575
Description: "All commands within the Dockerfile should use the same casing (either upper or lower)",
500576
URL: "https://docs.docker.com/go/dockerfile/rule/consistent-instruction-casing/",
501577
Detail: "Command 'frOM' should match the case of the command majority (lowercase)",
502-
Line: 3,
578+
Line: 4,
503579
Level: 1,
504580
},
505581
},
@@ -527,6 +603,7 @@ copy Dockerfile /bar
527603

528604
dockerfile = []byte(`
529605
# warning: 'from' should match command majority's casing (uppercase)
606+
#
530607
from scratch
531608
COPY Dockerfile /foo
532609
COPY Dockerfile /bar
@@ -540,14 +617,15 @@ COPY Dockerfile /baz
540617
Description: "All commands within the Dockerfile should use the same casing (either upper or lower)",
541618
URL: "https://docs.docker.com/go/dockerfile/rule/consistent-instruction-casing/",
542619
Detail: "Command 'from' should match the case of the command majority (uppercase)",
543-
Line: 3,
620+
Line: 4,
544621
Level: 1,
545622
},
546623
},
547624
})
548625

549626
dockerfile = []byte(`
550627
# warning: 'FROM' should match command majority's casing (lowercase)
628+
#
551629
FROM scratch
552630
copy Dockerfile /foo
553631
copy Dockerfile /bar
@@ -561,7 +639,7 @@ copy Dockerfile /baz
561639
Description: "All commands within the Dockerfile should use the same casing (either upper or lower)",
562640
URL: "https://docs.docker.com/go/dockerfile/rule/consistent-instruction-casing/",
563641
Detail: "Command 'FROM' should match the case of the command majority (lowercase)",
564-
Line: 3,
642+
Line: 4,
565643
Level: 1,
566644
},
567645
},
@@ -1382,6 +1460,12 @@ func checkUnmarshal(t *testing.T, sb integration.Sandbox, lintTest *lintTestPara
13821460
require.Less(t, lintResults.Error.Location.SourceIndex, int32(len(lintResults.Sources)))
13831461
}
13841462

1463+
if len(warnings) != len(lintResults.Warnings) {
1464+
for _, w := range lintResults.Warnings {
1465+
t.Logf("Warning Received: %s\n", w.Detail)
1466+
}
1467+
}
1468+
13851469
require.Equal(t, len(warnings), len(lintResults.Warnings))
13861470

13871471
sort.Slice(lintResults.Warnings, func(i, j int) bool {

‎frontend/dockerfile/docs/rules/_index.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,5 +103,9 @@ To learn more about how to use build checks, see
103103
<td><a href="./copy-ignored-file/">CopyIgnoredFile (experimental)</a></td>
104104
<td>Attempting to Copy file that is excluded by .dockerignore</td>
105105
</tr>
106+
<tr>
107+
<td><a href="./invalid-definition-description/">InvalidDefinitionDescription</a></td>
108+
<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>
109+
</tr>
106110
</tbody>
107111
</table>

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

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
---
2+
title: InvalidDefinitionDescription
3+
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.
4+
aliases:
5+
- /go/dockerfile/rule/invalid-definition-description/
6+
---
7+
8+
## Output
9+
10+
```text
11+
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.
12+
```
13+
14+
## Description
15+
16+
The [`--call=outline`](https://docs.docker.com/reference/cli/docker/buildx/build/#call-outline)
17+
and [`--call=targets`](https://docs.docker.com/reference/cli/docker/buildx/build/#call-outline)
18+
flags for the `docker build` command print descriptions for build targets and arguments.
19+
The descriptions are generated from [Dockerfile comments](https://docs.docker.com/reference/cli/docker/buildx/build/#descriptions)
20+
that immediately precede the `FROM` or `ARG` instruction
21+
and that begin with the name of the build stage or argument.
22+
For example:
23+
24+
```dockerfile
25+
# build-cli builds the CLI binary
26+
FROM alpine AS build-cli
27+
# VERSION controls the version of the program
28+
ARG VERSION=1
29+
```
30+
31+
In cases where preceding comments are not meant to be descriptions,
32+
add an empty line or comment between the instruction and the preceding comment.
33+
34+
## Examples
35+
36+
❌ Bad: A non-descriptive comment on the line preceding the `FROM` command.
37+
38+
```dockerfile
39+
# a non-descriptive comment
40+
FROM scratch AS base
41+
42+
# another non-descriptive comment
43+
ARG VERSION=1
44+
```
45+
46+
✅ Good: An empty line separating non-descriptive comments.
47+
48+
```dockerfile
49+
# a non-descriptive comment
50+
51+
FROM scratch AS base
52+
53+
# another non-descriptive comment
54+
55+
ARG VERSION=1
56+
```
57+
58+
✅ Good: Comments describing `ARG` keys and stages immediately proceeding the command.
59+
60+
```dockerfile
61+
# base is a stage for compiling source
62+
FROM scratch AS base
63+
# VERSION This is the version number.
64+
ARG VERSION=1
65+
```
66+

‎frontend/dockerfile/instructions/parse.go

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -100,7 +100,14 @@ func ParseInstructionWithLinter(node *parser.Node, lint *linter.Linter) (v inter
100100
msg := linter.RuleFromAsCasing.Format(req.command, req.args[1])
101101
lint.Run(&linter.RuleFromAsCasing, node.Location(), msg)
102102
}
103-
return parseFrom(req)
103+
fromCmd, err := parseFrom(req)
104+
if err != nil {
105+
return nil, err
106+
}
107+
if fromCmd.Name != "" {
108+
validateDefinitionDescription("FROM", []string{fromCmd.Name}, node.PrevComment, node.Location(), lint)
109+
}
110+
return fromCmd, nil
104111
case command.Onbuild:
105112
return parseOnBuild(req)
106113
case command.Workdir:
@@ -122,7 +129,16 @@ func ParseInstructionWithLinter(node *parser.Node, lint *linter.Linter) (v inter
122129
case command.StopSignal:
123130
return parseStopSignal(req)
124131
case command.Arg:
125-
return parseArg(req)
132+
argCmd, err := parseArg(req)
133+
if err != nil {
134+
return nil, err
135+
}
136+
argKeys := []string{}
137+
for _, arg := range argCmd.Args {
138+
argKeys = append(argKeys, arg.Key)
139+
}
140+
validateDefinitionDescription("ARG", argKeys, node.PrevComment, node.Location(), lint)
141+
return argCmd, nil
126142
case command.Shell:
127143
return parseShell(req)
128144
}
@@ -858,3 +874,22 @@ func doesFromCaseMatchAsCase(req parseRequest) bool {
858874
}
859875
return req.args[1] == strings.ToUpper(req.args[1])
860876
}
877+
878+
func validateDefinitionDescription(instruction string, argKeys []string, descComments []string, location []parser.Range, lint *linter.Linter) {
879+
if len(descComments) == 0 || len(argKeys) == 0 {
880+
return
881+
}
882+
descCommentParts := strings.Split(descComments[len(descComments)-1], " ")
883+
for _, key := range argKeys {
884+
if key == descCommentParts[0] {
885+
return
886+
}
887+
}
888+
exampleKey := argKeys[0]
889+
if len(argKeys) > 1 {
890+
exampleKey = "<arg_key>"
891+
}
892+
893+
msg := linter.RuleInvalidDefinitionDescription.Format(instruction, exampleKey)
894+
lint.Run(&linter.RuleInvalidDefinitionDescription, location, msg)
895+
}

0 commit comments

Comments
 (0)