Update file consistent cmd casing to match majority casing, not first…

… instruction casing Signed-off-by: Talon Bowler <[email protected]>
Mirantis · Mar 29, 2024 · c172ba7 · c172ba7
1 parent 30f3483
commit c172ba7
Show file tree

Hide file tree

Showing 3 changed files with 72 additions and 33 deletions.
diff --git a/frontend/dockerfile/dockerfile2llb/convert.go b/frontend/dockerfile/dockerfile2llb/convert.go
@@ -195,29 +195,7 @@ func toDispatchState(ctx context.Context, dt []byte, opt ConvertOpt) (*dispatchS
 		}
 	}
 
-	var firstConsistentCommand string
-	var isFirstCommandLower bool
-
-	for _, node := range dockerfile.AST.Children {
-		// 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
-		// first self-consistent command to every other command which is self-consistent.
-		if !isSelfConsistentCasing(node.Value) {
-			msg := linter.RuleSelfConsistentCommandCasing.Format(node.Value)
-			linter.RuleSelfConsistentCommandCasing.Run(opt.Warn, node.Location(), msg)
-		} else {
-			isCommandLowercase := strings.ToLower(node.Value) == node.Value
-			if firstConsistentCommand == "" {
-				firstConsistentCommand = node.Value
-				isFirstCommandLower = isCommandLowercase
-			} else {
-				if isCommandLowercase != isFirstCommandLower {
-					msg := linter.RuleFileConsistentCommandCasing.Format(firstConsistentCommand, node.Value)
-					linter.RuleFileConsistentCommandCasing.Run(opt.Warn, node.Location(), msg)
-				}
-			}
-		}
-	}
+	validateCommandCasing(dockerfile, opt.Warn)
 
 	proxyEnv := proxyEnvFromBuildArgs(opt.BuildArgs)
 
@@ -1967,3 +1945,40 @@ func isEnabledForStage(stage string, value string) bool {
 func isSelfConsistentCasing(s string) bool {
 	return s == strings.ToLower(s) || s == strings.ToUpper(s)
 }
+
+func validateCommandCasing(dockerfile *parser.Result, warn func(short, url string, detail [][]byte, location []parser.Range)) {
+	var lowerCount, upperCount int
+	for _, node := range dockerfile.AST.Children {
+		if isSelfConsistentCasing(node.Value) {
+			if strings.ToLower(node.Value) == node.Value {
+				lowerCount++
+			} else {
+				upperCount++
+			}
+		}
+	}
+
+	isMajorityLower := lowerCount > upperCount
+	for _, node := range dockerfile.AST.Children {
+		// 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.RuleSelfConsistentCommandCasing.Format(node.Value)
+			linter.RuleSelfConsistentCommandCasing.Run(warn, 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 {
+				linter.RuleFileConsistentCommandCasing.Run(warn, node.Location(), msg)
+			}
+		}
+	}
+}
diff --git a/frontend/dockerfile/dockerfile_test.go b/frontend/dockerfile/dockerfile_test.go
@@ -6856,31 +6856,57 @@ from scratch as base2
 func testFileConsistentCommandCasing(t *testing.T, sb integration.Sandbox) {
 	dockerfile := []byte(`
 FROM scratch
-
-# warning: 'copy' should match first command's casing (in this case, 'FROM's casing)
+# warning: 'copy' should match command majority's casing (uppercase)
 copy Dockerfile /foo
-
 COPY Dockerfile /bar
 `)
 	checkLintWarnings(t, sb, dockerfile, []expectedLintWarning{
 		{
-			Short:  "Lint Rule 'FileConsistentCommandCasing': Command 'copy' should be consistently cased with 'FROM' (line 5)",
+			Short:  "Lint Rule 'FileConsistentCommandCasing': Command 'copy' should match the case of the command majority (uppercase) (line 4)",
 			Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)",
 			Level:  1,
 		},
 	})
 
 	dockerfile = []byte(`
 from scratch
+# warning: 'COPY' should match command majority's casing (lowercase)
+COPY Dockerfile /foo
+copy Dockerfile /bar
+`)
+	checkLintWarnings(t, sb, dockerfile, []expectedLintWarning{
+		{
+			Short:  "Lint Rule 'FileConsistentCommandCasing': Command 'COPY' should match the case of the command majority (lowercase) (line 4)",
+			Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)",
+			Level:  1,
+		},
+	})
 
-# warning: 'COPY' should match first command's casing (in this case, 'from's casing)
+	dockerfile = []byte(`
+# warning: 'from' should match command majority's casing (uppercase)
+from scratch
 COPY Dockerfile /foo
+COPY Dockerfile /bar
+COPY Dockerfile /baz
+`)
+	checkLintWarnings(t, sb, dockerfile, []expectedLintWarning{
+		{
+			Short:  "Lint Rule 'FileConsistentCommandCasing': Command 'from' should match the case of the command majority (uppercase) (line 3)",
+			Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)",
+			Level:  1,
+		},
+	})
 
+	dockerfile = []byte(`
+# warning: 'FROM' should match command majority's casing (lowercase)
+FROM scratch
+copy Dockerfile /foo
 copy Dockerfile /bar
+copy Dockerfile /baz
 `)
 	checkLintWarnings(t, sb, dockerfile, []expectedLintWarning{
 		{
-			Short:  "Lint Rule 'FileConsistentCommandCasing': Command 'COPY' should be consistently cased with 'from' (line 5)",
+			Short:  "Lint Rule 'FileConsistentCommandCasing': Command 'FROM' should match the case of the command majority (lowercase) (line 3)",
 			Detail: "All commands within the Dockerfile should use the same casing (either upper or lower)",
 			Level:  1,
 		},
@@ -7079,8 +7105,6 @@ func checkLintWarnings(t *testing.T, sb integration.Sandbox, dockerfile []byte,
 		t.Fatalf("timed out waiting for statusDone")
 	}
 
-	<-statusDone
-
 	// two platforms only show one warning
 	require.Equal(t, len(expected), len(warnings))
 	sort.Slice(warnings, func(i, j int) bool {

diff --git a/frontend/dockerfile/linter/ruleset.go b/frontend/dockerfile/linter/ruleset.go
@@ -37,8 +37,8 @@ var (
 	RuleFileConsistentCommandCasing = LinterRule[func(string, string) string]{
 		Name:        "FileConsistentCommandCasing",
 		Description: "All commands within the Dockerfile should use the same casing (either upper or lower)",
-		Format: func(firstCommand, violatingCommand string) string {
-			return fmt.Sprintf("Command '%s' should be consistently cased with '%s'", violatingCommand, firstCommand)
+		Format: func(violatingCommand, correctCasing string) string {
+			return fmt.Sprintf("Command '%s' should match the case of the command majority (%s)", violatingCommand, correctCasing)
 		},
 	}
 )