PatchStrategy is not required on CRD conditions

Author: JoelSpeed

README.md

@@ -151,25 +151,34 @@ Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge"
 
 Conditions are idiomatically the first field within the status struct, and the linter will highlight when the Conditions are not the first field.
 
+Protobuf tags and patch strategy are required for in-tree API types, but not for CRDs.
+When linting CRD based types, set the `useProtobuf` and `usePatchStrategy` config option to `Ignore` or `Forbid`.
+
 ### Configuration
 
 ```yaml
 lintersConfig:
   conditions:
     isFirstField: Warn | Ignore # The policy for the Conditions field being the first field. Defaults to `Warn`.
-    useProtobuf: SuggestFix | Warn | Ignore # The policy for the protobuf tag on the Conditions field. Defaults to `SuggestFix`.
+    useProtobuf: SuggestFix | Warn | Ignore | Forbid # The policy for the protobuf tag on the Conditions field. Defaults to `SuggestFix`.
+    usePatchStrategy: SuggestFix | Warn | Ignore | Forbid # The policy for the patchStrategy tag on the Conditions field. Defaults to `SuggestFix`.
 ```
 
 ### Fixes (via standalone binary only)
 
 The `conditions` linter can automatically fix the tags on the `Conditions` field.
 When they do not match the expected format, the linter will suggest to update the tags to match the expected format.
 
-For CRDs, protobuf tags are not expected. By setting the `useProtobuf` configuration to `Ignore`, the linter will not suggest to add the protobuf tag to the `Conditions` field tags.
+For CRDs, protobuf tags and patch strategy are not expected.
+By setting the `useProtobuf`/`usePatchStrategy` configuration to `Ignore`, the linter will not suggest to add the protobuf/patch strategy tag to the `Conditions` field tags.
+By setting the `useProtobuf`/`usePatchStrategy` configuration to `Forbid`, the linter will suggest to remove the protobuf/patch strategy tag from the `Conditions` field tags.
 
 The linter will also suggest to add missing markers.
 If any of the 5 markers in the example above are missing, the linter will suggest to add them directly above the field.
 
+When `usePatchStrategy` is set to `Ignore`, the linter will not suggest to add the `patchStrategy` and `patchMergeKey` tags to the `Conditions` field markers.
+When `usePatchStrategy` is set to `Forbid`, the linter will suggest to remove the `patchStrategy` and `patchMergeKey` tags from the `Conditions` field markers.
+
 ## CommentStart
 
 The `commentstart` linter checks that all comments in the API types start with the serialized form of the type they are commenting on.

pkg/analysis/conditions/analyzer.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"go/ast"
 	"go/token"
+	"slices"
 	"strings"
 
 	"github.com/JoelSpeed/kal/pkg/analysis/helpers/extractjsontags"
@@ -23,14 +24,17 @@ const (
 	patchStrategyMarkerID = "patchStrategy"
 	patchMergeKeyMarkerID = "patchMergeKey"
 
-	listTypeMap       = "listType=map"
-	listMapKeyType    = "listMapKey=type"
-	patchStrategy     = "patchStrategy=merge"
-	patchMergeKeyType = "patchMergeKey=type"
-	optional          = "optional"
-
-	expectedTagWithProtobufFmt = "`json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\" protobuf:\"bytes,%d,rep,name=conditions\"`"
-	expectedTagWithoutProtobuf = "`json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\"`"
+	listTypeMap        = "listType=map"
+	listMapKeyType     = "listMapKey=type"
+	patchStrategy      = "patchStrategy"
+	patchStrategyMerge = "patchStrategy=merge"
+	patchMergeKey      = "patchMergeKey"
+	patchMergeKeyType  = "patchMergeKey=type"
+	optional           = "optional"
+
+	expectedJSONTag     = "json:\"conditions,omitempty\""
+	expectedPatchTag    = "patchStrategy:\"merge\" patchMergeKey:\"type\""
+	expectedProtobufTag = "protobuf:\"bytes,%d,rep,name=conditions\""
 )
 
 func init() {
@@ -49,17 +53,19 @@ var (
 )
 
 type analyzer struct {
-	isFirstField config.ConditionsFirstField
-	useProtobuf  config.ConditionsUseProtobuf
+	isFirstField     config.ConditionsFirstField
+	useProtobuf      config.ConditionsUseProtobuf
+	usePatchStrategy config.ConditionsUsePatchStrategy
 }
 
 // newAnalyzer creates a new analyzer.
 func newAnalyzer(cfg config.ConditionsConfig) *analysis.Analyzer {
 	defaultConfig(&cfg)
 
 	a := &analyzer{
-		isFirstField: cfg.IsFirstField,
-		useProtobuf:  cfg.UseProtobuf,
+		isFirstField:     cfg.IsFirstField,
+		useProtobuf:      cfg.UseProtobuf,
+		usePatchStrategy: cfg.UsePatchStrategy,
 	}
 
 	return &analysis.Analyzer{
@@ -117,16 +123,17 @@ func (a *analyzer) checkField(pass *analysis.Pass, index int, field *ast.Field,
 		return
 	}
 
-	checkFieldMarkers(pass, field, fieldMarkers)
+	checkFieldMarkers(pass, field, fieldMarkers, a.usePatchStrategy)
 	a.checkFieldTags(pass, index, field)
 
 	if a.isFirstField == config.ConditionsFirstFieldWarn && index != 0 {
 		pass.Reportf(field.Pos(), "Conditions field must be the first field in the struct")
 	}
 }
 
-func checkFieldMarkers(pass *analysis.Pass, field *ast.Field, fieldMarkers markers.MarkerSet) {
+func checkFieldMarkers(pass *analysis.Pass, field *ast.Field, fieldMarkers markers.MarkerSet, usePatchStrategy config.ConditionsUsePatchStrategy) {
 	missingMarkers := []string{}
+	additionalMarkers := []markers.Marker{}
 
 	if !fieldMarkers.HasWithValue(listTypeMap) {
 		missingMarkers = append(missingMarkers, listTypeMap)
@@ -136,37 +143,112 @@ func checkFieldMarkers(pass *analysis.Pass, field *ast.Field, fieldMarkers marke
 		missingMarkers = append(missingMarkers, listMapKeyType)
 	}
 
-	if !fieldMarkers.HasWithValue(patchStrategy) {
-		missingMarkers = append(missingMarkers, patchStrategy)
-	}
-
-	if !fieldMarkers.HasWithValue(patchMergeKeyType) {
-		missingMarkers = append(missingMarkers, patchMergeKeyType)
-	}
+	patchMissingMarkers, patchAdditionalMarkers := checkPatchStrategyMarkers(fieldMarkers, usePatchStrategy)
+	missingMarkers = append(missingMarkers, patchMissingMarkers...)
+	additionalMarkers = append(additionalMarkers, patchAdditionalMarkers...)
 
 	if !fieldMarkers.Has(optional) {
 		missingMarkers = append(missingMarkers, optional)
 	}
 
 	if len(missingMarkers) != 0 {
-		pass.Report(analysis.Diagnostic{
-			Pos:     field.Pos(),
-			End:     field.End(),
-			Message: "Conditions field is missing the following markers: " + strings.Join(missingMarkers, ", "),
-			SuggestedFixes: []analysis.SuggestedFix{
-				{
-					Message: "Add missing markers",
-					TextEdits: []analysis.TextEdit{
-						{
-							Pos:     field.Pos(),
-							End:     token.NoPos,
-							NewText: getNewMarkers(missingMarkers),
-						},
+		reportMissingMarkers(pass, field, missingMarkers, usePatchStrategy)
+	}
+
+	if len(additionalMarkers) != 0 {
+		reportAdditionalMarkers(pass, field, additionalMarkers)
+	}
+}
+
+func checkPatchStrategyMarkers(fieldMarkers markers.MarkerSet, usePatchStrategy config.ConditionsUsePatchStrategy) ([]string, []markers.Marker) {
+	missingMarkers := []string{}
+	additionalMarkers := []markers.Marker{}
+
+	switch usePatchStrategy {
+	case config.ConditionsUsePatchStrategySuggestFix, config.ConditionsUsePatchStrategyWarn:
+		if !fieldMarkers.HasWithValue(patchStrategyMerge) {
+			missingMarkers = append(missingMarkers, patchStrategyMerge)
+		}
+
+		if !fieldMarkers.HasWithValue(patchMergeKeyType) {
+			missingMarkers = append(missingMarkers, patchMergeKeyType)
+		}
+	case config.ConditionsUsePatchStrategyIgnore:
+		// If it's there, we don't care.
+	case config.ConditionsUsePatchStrategyForbid:
+		if fieldMarkers.HasWithValue(patchStrategyMerge) {
+			additionalMarkers = append(additionalMarkers, fieldMarkers[patchStrategy]...)
+		}
+
+		if fieldMarkers.HasWithValue(patchMergeKeyType) {
+			additionalMarkers = append(additionalMarkers, fieldMarkers[patchMergeKey]...)
+		}
+	default:
+		panic("unexpected usePatchStrategy value")
+	}
+
+	return missingMarkers, additionalMarkers
+}
+
+func reportMissingMarkers(pass *analysis.Pass, field *ast.Field, missingMarkers []string, usePatchStrategy config.ConditionsUsePatchStrategy) {
+	suggestedFixes := []analysis.SuggestedFix{}
+
+	// If patch strategy is warn, and the only markers in the list are patchStrategy and patchMergeKeyType, we don't need to suggest a fix.
+	if usePatchStrategy != config.ConditionsUsePatchStrategyWarn || slices.ContainsFunc[[]string, string](missingMarkers, func(marker string) bool {
+		switch marker {
+		case patchStrategyMerge, patchMergeKeyType:
+			return false
+		default:
+			return true
+		}
+	}) {
+		suggestedFixes = []analysis.SuggestedFix{
+			{
+				Message: "Add missing markers",
+				TextEdits: []analysis.TextEdit{
+					{
+						Pos:     field.Pos(),
+						End:     token.NoPos,
+						NewText: getNewMarkers(missingMarkers),
 					},
 				},
 			},
+		}
+	}
+
+	pass.Report(analysis.Diagnostic{
+		Pos:            field.Pos(),
+		End:            field.End(),
+		Message:        "Conditions field is missing the following markers: " + strings.Join(missingMarkers, ", "),
+		SuggestedFixes: suggestedFixes,
+	})
+}
+
+func reportAdditionalMarkers(pass *analysis.Pass, field *ast.Field, additionalMarkers []markers.Marker) {
+	suggestedFixes := []analysis.SuggestedFix{}
+	additionalMarkerValues := []string{}
+
+	for _, marker := range additionalMarkers {
+		additionalMarkerValues = append(additionalMarkerValues, marker.String())
+
+		suggestedFixes = append(suggestedFixes, analysis.SuggestedFix{
+			Message: "Remove additional marker",
+			TextEdits: []analysis.TextEdit{
+				{
+					Pos:     marker.Pos,
+					End:     marker.End + 1, // Add 1 to position to include the new line
+					NewText: nil,
+				},
+			},
 		})
 	}
+
+	pass.Report(analysis.Diagnostic{
+		Pos:            field.Pos(),
+		End:            field.End(),
+		Message:        "Conditions field has the following additional markers: " + strings.Join(additionalMarkerValues, ", "),
+		SuggestedFixes: suggestedFixes,
+	})
 }
 
 func getNewMarkers(missingMarkers []string) []byte {
@@ -181,7 +263,7 @@ func getNewMarkers(missingMarkers []string) []byte {
 
 func (a *analyzer) checkFieldTags(pass *analysis.Pass, index int, field *ast.Field) {
 	if field.Tag == nil {
-		expectedTag := getExpectedTag(a.useProtobuf, a.isFirstField, index)
+		expectedTag := getExpectedTag(a.usePatchStrategy, a.useProtobuf, a.isFirstField, index)
 
 		pass.Report(analysis.Diagnostic{
 			Pos:     field.Pos(),
@@ -204,9 +286,9 @@ func (a *analyzer) checkFieldTags(pass *analysis.Pass, index int, field *ast.Fie
 		return
 	}
 
-	asExpected, shouldFix := tagIsAsExpected(field.Tag.Value, a.useProtobuf, a.isFirstField, index)
+	asExpected, shouldFix := tagIsAsExpected(field.Tag.Value, a.usePatchStrategy, a.useProtobuf, a.isFirstField, index)
 	if !asExpected {
-		expectedTag := getExpectedTag(a.useProtobuf, a.isFirstField, index)
+		expectedTag := getExpectedTag(a.usePatchStrategy, a.useProtobuf, a.isFirstField, index)
 
 		if !shouldFix {
 			pass.Reportf(field.Tag.ValuePos, "Conditions field has incorrect tags, should be: %s", expectedTag)
@@ -232,27 +314,63 @@ func (a *analyzer) checkFieldTags(pass *analysis.Pass, index int, field *ast.Fie
 	}
 }
 
-func getExpectedTag(useProtobuf config.ConditionsUseProtobuf, isFirstField config.ConditionsFirstField, index int) string {
+func getExpectedTag(usePatchStrategy config.ConditionsUsePatchStrategy, useProtobuf config.ConditionsUseProtobuf, isFirstField config.ConditionsFirstField, index int) string {
+	expectedTag := fmt.Sprintf("`%s", expectedJSONTag)
+
+	if usePatchStrategy == config.ConditionsUsePatchStrategySuggestFix || usePatchStrategy == config.ConditionsUsePatchStrategyWarn {
+		expectedTag += fmt.Sprintf(" %s", expectedPatchTag)
+	}
+
 	if useProtobuf == config.ConditionsUseProtobufSuggestFix || useProtobuf == config.ConditionsUseProtobufWarn {
-		i := 1
-		if isFirstField == config.ConditionsFirstFieldIgnore {
-			i = index + 1
-		}
+		expectedTag += fmt.Sprintf(" %s", getExpectedProtobufTag(isFirstField, index))
+	}
 
-		return fmt.Sprintf(expectedTagWithProtobufFmt, i)
+	expectedTag += "`"
+
+	return expectedTag
+}
+
+func getExpectedProtobufTag(isFirstField config.ConditionsFirstField, index int) string {
+	i := 1
+	if isFirstField == config.ConditionsFirstFieldIgnore {
+		i = index + 1
 	}
 
-	return expectedTagWithoutProtobuf
+	return fmt.Sprintf(expectedProtobufTag, i)
+}
+
+func tagIsAsExpected(tag string, usePatchStrategy config.ConditionsUsePatchStrategy, useProtobuf config.ConditionsUseProtobuf, isFirstField config.ConditionsFirstField, index int) (bool, bool) {
+	patchTagCorrect, patchShouldSuggestFix := patchStrategyTagIsAsExpected(tag, usePatchStrategy)
+	protoTagCorrect, protoShouldSuggestFix := protobufTagIsAsExpected(tag, useProtobuf, isFirstField, index)
+
+	return patchTagCorrect && protoTagCorrect, patchShouldSuggestFix || protoShouldSuggestFix
+}
+
+func patchStrategyTagIsAsExpected(tag string, usePatchStrategy config.ConditionsUsePatchStrategy) (bool, bool) {
+	switch usePatchStrategy {
+	case config.ConditionsUsePatchStrategySuggestFix:
+		return strings.Contains(tag, expectedPatchTag), true
+	case config.ConditionsUsePatchStrategyWarn:
+		return strings.Contains(tag, expectedPatchTag), false
+	case config.ConditionsUsePatchStrategyIgnore:
+		return true, false
+	case config.ConditionsUsePatchStrategyForbid:
+		return !strings.Contains(tag, expectedPatchTag), true
+	default:
+		panic("unexpected usePatchStrategy value")
+	}
 }
 
-func tagIsAsExpected(tag string, useProtobuf config.ConditionsUseProtobuf, isFirstField config.ConditionsFirstField, index int) (bool, bool) {
+func protobufTagIsAsExpected(tag string, useProtobuf config.ConditionsUseProtobuf, isFirstField config.ConditionsFirstField, index int) (bool, bool) {
 	switch useProtobuf {
 	case config.ConditionsUseProtobufSuggestFix:
-		return tag == getExpectedTag(config.ConditionsUseProtobufSuggestFix, isFirstField, index), true
+		return strings.Contains(tag, getExpectedProtobufTag(isFirstField, index)), true
 	case config.ConditionsUseProtobufWarn:
-		return tag == getExpectedTag(config.ConditionsUseProtobufWarn, isFirstField, index), false
+		return strings.Contains(tag, getExpectedProtobufTag(isFirstField, index)), false
 	case config.ConditionsUseProtobufIgnore:
-		return tag == getExpectedTag(config.ConditionsUseProtobufIgnore, isFirstField, index) || tag == getExpectedTag(config.ConditionsUseProtobufSuggestFix, isFirstField, index), true
+		return true, false
+	case config.ConditionsUseProtobufForbid:
+		return !strings.Contains(tag, getExpectedProtobufTag(isFirstField, index)), true
 	default:
 		panic("unexpected useProtobuf value")
 	}
@@ -309,4 +427,8 @@ func defaultConfig(cfg *config.ConditionsConfig) {
 	if cfg.UseProtobuf == "" {
 		cfg.UseProtobuf = config.ConditionsUseProtobufSuggestFix
 	}
+
+	if cfg.UsePatchStrategy == "" {
+		cfg.UsePatchStrategy = config.ConditionsUsePatchStrategySuggestFix
+	}
 }