Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate instruction casing lint rules #5046

Merged
merged 1 commit into from
Jun 17, 2024
Merged

Consolidate instruction casing lint rules #5046

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 8 additions & 15 deletions frontend/dockerfile/dockerfile2llb/convert.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2162,22 +2162,15 @@ func validateCommandCasing(dockerfile *parser.Result, lint *linter.Linter) {
// Here, we check both if the command is consistent per command (ie, "CMD" or "cmd", not "Cmd")
// as well as ensuring that the casing is consistent throughout the dockerfile by comparing the
// command to the casing of the majority of commands.
if !isSelfConsistentCasing(node.Value) {
msg := linter.RuleConsistentInstructionCasing.Format(node.Value)
var correctCasing string
if isMajorityLower && strings.ToLower(node.Value) != node.Value {
correctCasing = "lowercase"
} else if !isMajorityLower && strings.ToUpper(node.Value) != node.Value {
correctCasing = "uppercase"
}
if correctCasing != "" {
msg := linter.RuleConsistentInstructionCasing.Format(node.Value, correctCasing)
lint.Run(&linter.RuleConsistentInstructionCasing, node.Location(), msg)
} else {
var msg string
var needsLintWarn bool
if isMajorityLower && strings.ToUpper(node.Value) == node.Value {
msg = linter.RuleFileConsistentCommandCasing.Format(node.Value, "lowercase")
needsLintWarn = true
} else if !isMajorityLower && strings.ToLower(node.Value) == node.Value {
msg = linter.RuleFileConsistentCommandCasing.Format(node.Value, "uppercase")
needsLintWarn = true
}
if needsLintWarn {
lint.Run(&linter.RuleFileConsistentCommandCasing, node.Location(), msg)
}
}
}
}
Expand Down
61 changes: 29 additions & 32 deletions frontend/dockerfile/dockerfile_lint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ var lintTests = integration.TestFuncs(
testStageName,
testNoEmptyContinuation,
testConsistentInstructionCasing,
testFileConsistentCommandCasing,
testDuplicateStageName,
testReservedStageName,
testJSONArgsRecommended,
Expand Down Expand Up @@ -97,19 +96,19 @@ copy Dockerfile .
`)
checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})

dockerfile = []byte(`#check=skip=FileConsistentCommandCasing,FromAsCasing
dockerfile = []byte(`#check=skip=ConsistentInstructionCasing,FromAsCasing
FROM scratch as base
copy Dockerfile .
`)
checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})

dockerfile = []byte(`#check=skip=FileConsistentCommandCasing,FromAsCasing;error=true
dockerfile = []byte(`#check=skip=ConsistentInstructionCasing,FromAsCasing;error=true
FROM scratch as base
copy Dockerfile .
`)
checkLinterWarnings(t, sb, &lintTestParams{Dockerfile: dockerfile})

dockerfile = []byte(`#check=skip=FileConsistentCommandCasing
dockerfile = []byte(`#check=skip=ConsistentInstructionCasing
FROM scratch as base
copy Dockerfile .
`)
Expand All @@ -127,7 +126,7 @@ copy Dockerfile .
},
})

dockerfile = []byte(`#check=skip=FileConsistentCommandCasing;error=true
dockerfile = []byte(`#check=skip=ConsistentInstructionCasing;error=true
FROM scratch as base
copy Dockerfile .
`)
Expand Down Expand Up @@ -168,7 +167,7 @@ copy Dockerfile .
UnmarshalBuildErr: "lint violation found for rules: FromAsCasing",
BuildErrLocation: 2,
FrontendAttrs: map[string]string{
"build-arg:BUILDKIT_DOCKERFILE_CHECK": "skip=FileConsistentCommandCasing;error=true",
"build-arg:BUILDKIT_DOCKERFILE_CHECK": "skip=ConsistentInstructionCasing;error=true",
},
})

Expand Down Expand Up @@ -275,52 +274,50 @@ FROM scratch AS base2
Warnings: []expectedLintWarning{
{
RuleName: "ConsistentInstructionCasing",
Description: "Instructions should be in consistent casing (all lower or all upper)",
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 be consistently cased",
Detail: "Command 'From' should match the case of the command majority (uppercase)",
Level: 1,
Line: 3,
},
},
})

dockerfile = []byte(`
# warning: 'FROM' should be either lowercased or uppercased
frOM scratch as base
from scratch as base2
FROM scratch
# warning: 'copy' should match command majority's casing (uppercase)
copy Dockerfile /foo
COPY Dockerfile /bar
`)

checkLinterWarnings(t, sb, &lintTestParams{
Dockerfile: dockerfile,
Warnings: []expectedLintWarning{
{
RuleName: "ConsistentInstructionCasing",
Description: "Instructions should be in consistent casing (all lower or all upper)",
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 be consistently cased",
Line: 3,
Detail: "Command 'copy' should match the case of the command majority (uppercase)",
Line: 4,
Level: 1,
},
},
})
}

func testFileConsistentCommandCasing(t *testing.T, sb integration.Sandbox) {
dockerfile := []byte(`
FROM scratch
# warning: 'copy' should match command majority's casing (uppercase)
copy Dockerfile /foo
COPY Dockerfile /bar
dockerfile = []byte(`
# warning: 'frOM' should be either lowercased or uppercased
frOM scratch as base
from scratch as base2
`)

checkLinterWarnings(t, sb, &lintTestParams{
Dockerfile: dockerfile,
Warnings: []expectedLintWarning{
{
RuleName: "FileConsistentCommandCasing",
RuleName: "ConsistentInstructionCasing",
Description: "All commands within the Dockerfile should use the same casing (either upper or lower)",
URL: "https://docs.docker.com/go/dockerfile/rule/file-consistent-command-casing/",
Detail: "Command 'copy' should match the case of the command majority (uppercase)",
Line: 4,
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,
Level: 1,
},
},
Expand All @@ -336,9 +333,9 @@ copy Dockerfile /bar
Dockerfile: dockerfile,
Warnings: []expectedLintWarning{
{
RuleName: "FileConsistentCommandCasing",
RuleName: "ConsistentInstructionCasing",
Description: "All commands within the Dockerfile should use the same casing (either upper or lower)",
URL: "https://docs.docker.com/go/dockerfile/rule/file-consistent-command-casing/",
URL: "https://docs.docker.com/go/dockerfile/rule/consistent-instruction-casing/",
Detail: "Command 'COPY' should match the case of the command majority (lowercase)",
Line: 4,
Level: 1,
Expand All @@ -357,9 +354,9 @@ COPY Dockerfile /baz
Dockerfile: dockerfile,
Warnings: []expectedLintWarning{
{
RuleName: "FileConsistentCommandCasing",
RuleName: "ConsistentInstructionCasing",
Description: "All commands within the Dockerfile should use the same casing (either upper or lower)",
URL: "https://docs.docker.com/go/dockerfile/rule/file-consistent-command-casing/",
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,
Level: 1,
Expand All @@ -378,9 +375,9 @@ copy Dockerfile /baz
Dockerfile: dockerfile,
Warnings: []expectedLintWarning{
{
RuleName: "FileConsistentCommandCasing",
RuleName: "ConsistentInstructionCasing",
Description: "All commands within the Dockerfile should use the same casing (either upper or lower)",
URL: "https://docs.docker.com/go/dockerfile/rule/file-consistent-command-casing/",
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,
Level: 1,
Expand Down
4 changes: 0 additions & 4 deletions frontend/dockerfile/docs/rules/_index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,6 @@ $ docker build --check .
</tr>
<tr>
<td><a href="./consistent-instruction-casing/">ConsistentInstructionCasing</a></td>
<td>Instructions should be in consistent casing (all lower or all upper)</td>
</tr>
<tr>
<td><a href="./file-consistent-command-casing/">FileConsistentCommandCasing</a></td>
<td>All commands within the Dockerfile should use the same casing (either upper or lower)</td>
</tr>
<tr>
Expand Down
2 changes: 1 addition & 1 deletion frontend/dockerfile/docs/rules/consistent-instruction-casing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
title: ConsistentInstructionCasing
description: Instructions should be in consistent casing (all lower or all upper)
description: All commands within the Dockerfile should use the same casing (either upper or lower)
aliases:
- /go/dockerfile/rule/consistent-instruction-casing/
---
Expand Down
61 changes: 0 additions & 61 deletions frontend/dockerfile/docs/rules/file-consistent-command-casing.md
View file
Open in desktop

This file was deleted.

53 changes: 0 additions & 53 deletions frontend/dockerfile/linter/docs/FileConsistentCommandCasing.md
View file
Open in desktop

This file was deleted.

12 changes: 2 additions & 10 deletions frontend/dockerfile/linter/ruleset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,10 @@ var (
return "Empty continuation line"
},
}
RuleConsistentInstructionCasing = LinterRule[func(string) string]{
RuleConsistentInstructionCasing = LinterRule[func(string, string) string]{
Name: "ConsistentInstructionCasing",
Description: "Instructions should be in consistent casing (all lower or all upper)",
URL: "https://docs.docker.com/go/dockerfile/rule/consistent-instruction-casing/",
Format: func(command string) string {
return fmt.Sprintf("Command '%s' should be consistently cased", command)
},
}
RuleFileConsistentCommandCasing = LinterRule[func(string, string) string]{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not needed yet, but generally I think we need to keep old rules/URLs in here so the docs are still generated for them. Otherwise old client will hit issue, click on a link and get 404

Copy link
Collaborator Author

@daghack daghack Jun 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good thought to bring up, and you're definitely right. There may be a case for adding some sort of Deprecated flag to the ruleset so that documentation can automatically note it as such as follow-up work.

Name: "FileConsistentCommandCasing",
Description: "All commands within the Dockerfile should use the same casing (either upper or lower)",
URL: "https://docs.docker.com/go/dockerfile/rule/file-consistent-command-casing/",
URL: "https://docs.docker.com/go/dockerfile/rule/consistent-instruction-casing/",
Format: func(violatingCommand, correctCasing string) string {
return fmt.Sprintf("Command '%s' should match the case of the command majority (%s)", violatingCommand, correctCasing)
},
Expand Down
Loading