Add Conditions linter to verify conditions type, tags and markers

Author: JoelSpeed

README.md

@@ -128,6 +128,41 @@ allowing for customisation or automatic copmilation of the project should it not
 
 # Linters
 
+## Conditions
+
+The `conditions` linter checks that `Conditions` fields in the API types are correctly formatted.
+The `Conditions` field should be a slice of `metav1.Condition` with the following tags and markers:
+
+```go
+// +listType=map
+// +listMapKey=type
+// +patchStrategy=merge
+// +patchMergeKey=type
+// +optional
+Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,opt,name=conditions"`
+```
+
+Conditions are idiomatically the first field within the status struct, and the linter will highlight when the Conditions are not the first field.
+
+### 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`.
+```
+
+### 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.
+
+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.
+
 ## 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

@@ -0,0 +1,301 @@
+package conditions
+
+import (
+	"errors"
+	"fmt"
+	"go/ast"
+	"go/token"
+	"strings"
+
+	"github.com/JoelSpeed/kal/pkg/analysis/helpers/extractjsontags"
+	"github.com/JoelSpeed/kal/pkg/analysis/helpers/markers"
+	"github.com/JoelSpeed/kal/pkg/config"
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+const (
+	name = "conditions"
+
+	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\"`"
+)
+
+var (
+	errCouldNotGetInspector = errors.New("could not get inspector")
+	errCouldNotGetMarkers   = errors.New("could not get markers")
+)
+
+type analyzer struct {
+	isFirstField config.ConditionsFirstField
+	useProtobuf  config.ConditionsUseProtobuf
+}
+
+// newAnalyzer creates a new analyzer.
+func newAnalyzer(cfg config.ConditionsConfig) *analysis.Analyzer {
+	defaultConfig(&cfg)
+
+	a := &analyzer{
+		isFirstField: cfg.IsFirstField,
+		useProtobuf:  cfg.UseProtobuf,
+	}
+
+	return &analysis.Analyzer{
+		Name:     name,
+		Doc:      `Checks that all conditions type fields conform to the required conventions.`,
+		Run:      a.run,
+		Requires: []*analysis.Analyzer{inspect.Analyzer, markers.Analyzer, extractjsontags.Analyzer},
+	}
+}
+
+func (a *analyzer) run(pass *analysis.Pass) (interface{}, error) {
+	inspect, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+	if !ok {
+		return nil, errCouldNotGetInspector
+	}
+
+	markersAccess, ok := pass.ResultOf[markers.Analyzer].(markers.Markers)
+	if !ok {
+		return nil, errCouldNotGetMarkers
+	}
+
+	// Filter to structs so that we can iterate over fields in a struct.
+	nodeFilter := []ast.Node{
+		(*ast.StructType)(nil),
+	}
+
+	inspect.Preorder(nodeFilter, func(n ast.Node) {
+		sTyp, ok := n.(*ast.StructType)
+		if !ok {
+			return
+		}
+
+		if sTyp.Fields == nil {
+			return
+		}
+
+		for i, field := range sTyp.Fields.List {
+			if field == nil || len(field.Names) == 0 {
+				continue
+			}
+
+			fieldName := field.Names[0].Name
+			fieldMarkers := markersAccess.StructFieldMarkers(sTyp, fieldName)
+
+			a.checkField(pass, i, field, fieldMarkers)
+		}
+	})
+
+	return nil, nil //nolint:nilnil
+}
+
+func (a *analyzer) checkField(pass *analysis.Pass, index int, field *ast.Field, fieldMarkers markers.MarkerSet) {
+	if !fieldIsCalledConditions(field) {
+		return
+	}
+
+	if !isSliceMetaV1Condition(field) {
+		pass.Reportf(field.Pos(), "Conditions field must be a slice of metav1.Condition")
+		return
+	}
+
+	checkFieldMarkers(pass, field, fieldMarkers)
+	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) {
+	missingMarkers := []string{}
+
+	if !fieldMarkers.Has(listTypeMap) {
+		missingMarkers = append(missingMarkers, listTypeMap)
+	}
+
+	if !fieldMarkers.Has(listMapKeyType) {
+		missingMarkers = append(missingMarkers, listMapKeyType)
+	}
+
+	if !fieldMarkers.Has(patchStrategy) {
+		missingMarkers = append(missingMarkers, patchStrategy)
+	}
+
+	if !fieldMarkers.Has(patchMergeKeyType) {
+		missingMarkers = append(missingMarkers, patchMergeKeyType)
+	}
+
+	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),
+						},
+					},
+				},
+			},
+		})
+	}
+}
+
+func getNewMarkers(missingMarkers []string) []byte {
+	var out string
+
+	for _, marker := range missingMarkers {
+		out += "// +" + marker + "\n"
+	}
+
+	return []byte(out)
+}
+
+func (a *analyzer) checkFieldTags(pass *analysis.Pass, index int, field *ast.Field) {
+	if field.Tag == nil {
+		expectedTag := getExpectedTag(a.useProtobuf, a.isFirstField, index)
+
+		pass.Report(analysis.Diagnostic{
+			Pos:     field.Pos(),
+			End:     field.End(),
+			Message: "Conditions field is missing tags, should be: " + expectedTag,
+			SuggestedFixes: []analysis.SuggestedFix{
+				{
+					Message: "Add missing tags",
+					TextEdits: []analysis.TextEdit{
+						{
+							Pos:     field.End(),
+							End:     token.NoPos,
+							NewText: []byte(expectedTag),
+						},
+					},
+				},
+			},
+		})
+
+		return
+	}
+
+	asExpected, shouldFix := tagIsAsExpected(field.Tag.Value, a.useProtobuf, a.isFirstField, index)
+	if !asExpected {
+		expectedTag := getExpectedTag(a.useProtobuf, a.isFirstField, index)
+
+		if !shouldFix {
+			pass.Reportf(field.Tag.ValuePos, "Conditions field has incorrect tags, should be: %s", expectedTag)
+		} else {
+			pass.Report(analysis.Diagnostic{
+				Pos:     field.Tag.ValuePos,
+				End:     field.Tag.End(),
+				Message: "Conditions field has incorrect tags, should be: " + expectedTag,
+				SuggestedFixes: []analysis.SuggestedFix{
+					{
+						Message: "Update tags",
+						TextEdits: []analysis.TextEdit{
+							{
+								Pos:     field.Tag.ValuePos,
+								End:     field.Tag.End(),
+								NewText: []byte(expectedTag),
+							},
+						},
+					},
+				},
+			})
+		}
+	}
+}
+
+func getExpectedTag(useProtobuf config.ConditionsUseProtobuf, isFirstField config.ConditionsFirstField, index int) string {
+	if useProtobuf == config.ConditionsUseProtobufSuggestFix || useProtobuf == config.ConditionsUseProtobufWarn {
+		i := 1
+		if isFirstField == config.ConditionsFirstFieldIgnore {
+			i = index + 1
+		}
+
+		return fmt.Sprintf(expectedTagWithProtobufFmt, i)
+	}
+
+	return expectedTagWithoutProtobuf
+}
+
+func tagIsAsExpected(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
+	case config.ConditionsUseProtobufWarn:
+		return tag == getExpectedTag(config.ConditionsUseProtobufWarn, isFirstField, index), false
+	case config.ConditionsUseProtobufIgnore:
+		return tag == getExpectedTag(config.ConditionsUseProtobufIgnore, isFirstField, index) || tag == getExpectedTag(config.ConditionsUseProtobufSuggestFix, isFirstField, index), true
+	default:
+		panic("unexpected useProtobuf value")
+	}
+}
+
+func fieldIsCalledConditions(field *ast.Field) bool {
+	if field == nil {
+		return false
+	}
+
+	return len(field.Names) != 0 && field.Names[0] != nil && field.Names[0].Name == "Conditions"
+}
+
+func isSliceMetaV1Condition(field *ast.Field) bool {
+	if field == nil {
+		return false
+	}
+
+	// Field is not an array type.
+	arr, ok := field.Type.(*ast.ArrayType)
+	if !ok {
+		return false
+	}
+
+	// Array element is not imported.
+	selector, ok := arr.Elt.(*ast.SelectorExpr)
+	if !ok {
+		return false
+	}
+
+	pkg, ok := selector.X.(*ast.Ident)
+	if !ok {
+		return false
+	}
+
+	// Array element is not imported from metav1.
+	if selector.X == nil || pkg.Name != "metav1" {
+		return false
+	}
+
+	// Array element is not a metav1.Condition.
+	if selector.Sel == nil || selector.Sel.Name != "Condition" {
+		return false
+	}
+
+	return true
+}
+
+func defaultConfig(cfg *config.ConditionsConfig) {
+	if cfg.IsFirstField == "" {
+		cfg.IsFirstField = config.ConditionsFirstFieldWarn
+	}
+
+	if cfg.UseProtobuf == "" {
+		cfg.UseProtobuf = config.ConditionsUseProtobufSuggestFix
+	}
+}

pkg/analysis/conditions/analyzer_test.go

@@ -0,0 +1,50 @@
+package conditions_test
+
+import (
+	"testing"
+
+	"github.com/JoelSpeed/kal/pkg/analysis/conditions"
+	"github.com/JoelSpeed/kal/pkg/config"
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestDefaultConfiguration(t *testing.T) {
+	testdata := analysistest.TestData()
+
+	a, err := conditions.Initializer().Init(config.LintersConfig{})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	analysistest.RunWithSuggestedFixes(t, testdata, a, "a")
+}
+
+func TestNotFieldFirst(t *testing.T) {
+	testdata := analysistest.TestData()
+
+	a, err := conditions.Initializer().Init(config.LintersConfig{
+		Conditions: config.ConditionsConfig{
+			IsFirstField: config.ConditionsFirstFieldIgnore,
+		},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	analysistest.RunWithSuggestedFixes(t, testdata, a, "b")
+}
+
+func TestIgnoreProtobuf(t *testing.T) {
+	testdata := analysistest.TestData()
+
+	a, err := conditions.Initializer().Init(config.LintersConfig{
+		Conditions: config.ConditionsConfig{
+			UseProtobuf: config.ConditionsUseProtobufIgnore,
+		},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	analysistest.RunWithSuggestedFixes(t, testdata, a, "c")
+}