bugfix: stop skipping bools in linter, and throw on non-prefixed comm…

…ent interjected
kubernetes · Jan 11, 2024 · 7beda1b · 7beda1b
1 parent f0debae
commit 7beda1b
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 6 deletions.
diff --git a/pkg/generators/markers.go b/pkg/generators/markers.go
@@ -234,18 +234,25 @@ func ParseCommentTags(t *types.Type, comments []string, prefix string) (CommentT
 // lintArrayIndices attempts to prevent:
 // 1. Skipped indices
 // 2. Non-consecutive indices
-func lintArrayIndices(markerComments []string) error {
+func lintArrayIndices(markerComments []string, prefix string) error {
 	var lintErrors []error
 	prevLine := ""
 	prevIndex := -1
 	for _, line := range markerComments {
 		line = strings.Trim(line, " ")
-		split := strings.Split(line, "=")
-		if len(split) < 2 {
-			// Ignore malformed key in linter.
+
+		// If there was a non-prefixed marker inserted in between, then
+		// reset the previous line and index.
+		if !strings.HasPrefix(line, "+"+prefix) {
+			prevLine = ""
+			prevIndex = -1
 			continue
 		}
-		key := split[0]
+
+		// There will always be at least the first element in the split.
+		// If there is no = sign, we want to use the whole line as the key in the
+		// case of boolean marker comments.
+		key := strings.Split(line, "=")[0]
 		subscriptIdx := strings.Index(key, "[")
 		if subscriptIdx == -1 {
 			prevLine = ""
@@ -291,7 +298,7 @@ func lintArrayIndices(markerComments []string) error {
 // The prefix is removed from the key in the returned map.
 // Empty keys and invalid values will return errors, refs are currently unsupported and will be skipped.
 func parseMarkers(markerComments []string, prefix string) (map[string]any, error) {
-	if err := lintArrayIndices(markerComments); err != nil {
+	if err := lintArrayIndices(markerComments, prefix); err != nil {
 		return nil, err
 	}
 

diff --git a/pkg/generators/markers_test.go b/pkg/generators/markers_test.go
@@ -264,6 +264,67 @@ func TestParseCommentTags(t *testing.T) {
 			},
 			expectedError: `failed to parse marker comments: error parsing +k8s:validation:cel[2]:rule="a rule": non-consecutive index 2 for key '+k8s:validation:cel'`,
 		},
+		{
+			t:    types.Float64,
+			name: "interjected array index with non-prefixed comment",
+			comments: []string{
+				`+k8s:validation:cel[0]:rule="oldSelf == self"`,
+				`+k8s:validation:cel[0]:message="cant change"`,
+				`+k8s:validation:cel[1]:rule="self > 5"`,
+				`+k8s:validation:cel[1]:message="must be greater than 5"`,
+				`+minimum=5`,
+				`+k8s:validation:cel[2]:rule="a rule"`,
+				`+k8s:validation:cel[2]:message="message 2"`,
+			},
+			expectedError: `failed to parse marker comments: error parsing +k8s:validation:cel[2]:rule="a rule": non-consecutive index 2 for key '+k8s:validation:cel'`,
+		},
+		{
+			t:    types.Float64,
+			name: "boolean key at invalid index",
+			comments: []string{
+				`+k8s:validation:cel[0]:rule="oldSelf == self"`,
+				`+k8s:validation:cel[0]:message="cant change"`,
+				`+k8s:validation:cel[2]:optionalOldSelf`,
+			},
+			expectedError: `failed to parse marker comments: error parsing +k8s:validation:cel[2]:optionalOldSelf: non-consecutive index 2 for key '+k8s:validation:cel'`,
+		},
+		{
+			t:    types.Float64,
+			name: "boolean key after non-prefixed comment",
+			comments: []string{
+				`+k8s:validation:cel[0]:rule="oldSelf == self"`,
+				`+k8s:validation:cel[0]:message="cant change"`,
+				`+k8s:validation:cel[1]:rule="self > 5"`,
+				`+k8s:validation:cel[1]:message="must be greater than 5"`,
+				`+minimum=5`,
+				`+k8s:validation:cel[1]:optionalOldSelf`,
+			},
+			expectedError: `failed to parse marker comments: error parsing +k8s:validation:cel[1]:optionalOldSelf: non-consecutive index 1 for key '+k8s:validation:cel'`,
+		},
+		{
+			t:    types.Float64,
+			name: "boolean key at index allowed",
+			comments: []string{
+				`+k8s:validation:cel[0]:rule="oldSelf == self"`,
+				`+k8s:validation:cel[0]:message="cant change"`,
+				`+k8s:validation:cel[1]:rule="self > 5"`,
+				`+k8s:validation:cel[1]:message="must be greater than 5"`,
+				`+k8s:validation:cel[1]:optionalOldSelf`,
+			},
+			expected: generators.CommentTags{
+				CEL: []generators.CELTag{
+					{
+						Rule:    "oldSelf == self",
+						Message: "cant change",
+					},
+					{
+						Rule:            "self > 5",
+						Message:         "must be greater than 5",
+						OptionalOldSelf: ptr.To(true),
+					},
+				},
+			},
+		},
 	}
 
 	for _, tc := range cases {