Skip to content

Commit a6a6330

Browse files
JoelSpeedJoelSpeed
committed
PatchStrategy is not required on CRD conditions
1 parent cece271 commit a6a6330

File tree

12 files changed

+799
-51
lines changed

12 files changed

+799
-51
lines changed

‎README.md

+11-2
Original file line numberDiff line numberDiff line change
@@ -151,25 +151,34 @@ Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge"
151151

152152
Conditions are idiomatically the first field within the status struct, and the linter will highlight when the Conditions are not the first field.
153153

154+
Protobuf tags and patch strategy are required for in-tree API types, but not for CRDs.
155+
When linting CRD based types, set the `useProtobuf` and `usePatchStrategy` config option to `Ignore` or `Forbid`.
156+
154157
### Configuration
155158

156159
```yaml
157160
lintersConfig:
158161
conditions:
159162
isFirstField: Warn | Ignore # The policy for the Conditions field being the first field. Defaults to `Warn`.
160-
useProtobuf: SuggestFix | Warn | Ignore # The policy for the protobuf tag on the Conditions field. Defaults to `SuggestFix`.
163+
useProtobuf: SuggestFix | Warn | Ignore | Forbid # The policy for the protobuf tag on the Conditions field. Defaults to `SuggestFix`.
164+
usePatchStrategy: SuggestFix | Warn | Ignore | Forbid # The policy for the patchStrategy tag on the Conditions field. Defaults to `SuggestFix`.
161165
```
162166
163167
### Fixes (via standalone binary only)
164168
165169
The `conditions` linter can automatically fix the tags on the `Conditions` field.
166170
When they do not match the expected format, the linter will suggest to update the tags to match the expected format.
167171

168-
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.
172+
For CRDs, protobuf tags and patch strategy are not expected.
173+
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.
174+
By setting the `useProtobuf`/`usePatchStrategy` configuration to `Forbid`, the linter will suggest to remove the protobuf/patch strategy tag from the `Conditions` field tags.
169175

170176
The linter will also suggest to add missing markers.
171177
If any of the 5 markers in the example above are missing, the linter will suggest to add them directly above the field.
172178

179+
When `usePatchStrategy` is set to `Ignore`, the linter will not suggest to add the `patchStrategy` and `patchMergeKey` tags to the `Conditions` field markers.
180+
When `usePatchStrategy` is set to `Forbid`, the linter will suggest to remove the `patchStrategy` and `patchMergeKey` tags from the `Conditions` field markers.
181+
173182
## CommentStart
174183

175184
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

+156-46
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55
"fmt"
66
"go/ast"
77
"go/token"
8+
"slices"
89
"strings"
910

1011
"github.com/JoelSpeed/kal/pkg/analysis/helpers/extractjsontags"
@@ -23,14 +24,17 @@ const (
2324
patchStrategyMarkerID = "patchStrategy"
2425
patchMergeKeyMarkerID = "patchMergeKey"
2526

26-
listTypeMap = "listType=map"
27-
listMapKeyType = "listMapKey=type"
28-
patchStrategy = "patchStrategy=merge"
29-
patchMergeKeyType = "patchMergeKey=type"
30-
optional = "optional"
31-
32-
expectedTagWithProtobufFmt = "`json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\" protobuf:\"bytes,%d,rep,name=conditions\"`"
33-
expectedTagWithoutProtobuf = "`json:\"conditions,omitempty\" patchStrategy:\"merge\" patchMergeKey:\"type\"`"
27+
listTypeMap = "listType=map"
28+
listMapKeyType = "listMapKey=type"
29+
patchStrategy = "patchStrategy"
30+
patchStrategyMerge = "patchStrategy=merge"
31+
patchMergeKey = "patchMergeKey"
32+
patchMergeKeyType = "patchMergeKey=type"
33+
optional = "optional"
34+
35+
expectedJSONTag = "json:\"conditions,omitempty\""
36+
expectedPatchTag = "patchStrategy:\"merge\" patchMergeKey:\"type\""
37+
expectedProtobufTag = "protobuf:\"bytes,%d,rep,name=conditions\""
3438
)
3539

3640
func init() {
@@ -49,17 +53,19 @@ var (
4953
)
5054

5155
type analyzer struct {
52-
isFirstField config.ConditionsFirstField
53-
useProtobuf config.ConditionsUseProtobuf
56+
isFirstField config.ConditionsFirstField
57+
useProtobuf config.ConditionsUseProtobuf
58+
usePatchStrategy config.ConditionsUsePatchStrategy
5459
}
5560

5661
// newAnalyzer creates a new analyzer.
5762
func newAnalyzer(cfg config.ConditionsConfig) *analysis.Analyzer {
5863
defaultConfig(&cfg)
5964

6065
a := &analyzer{
61-
isFirstField: cfg.IsFirstField,
62-
useProtobuf: cfg.UseProtobuf,
66+
isFirstField: cfg.IsFirstField,
67+
useProtobuf: cfg.UseProtobuf,
68+
usePatchStrategy: cfg.UsePatchStrategy,
6369
}
6470

6571
return &analysis.Analyzer{
@@ -117,16 +123,17 @@ func (a *analyzer) checkField(pass *analysis.Pass, index int, field *ast.Field,
117123
return
118124
}
119125

120-
checkFieldMarkers(pass, field, fieldMarkers)
126+
checkFieldMarkers(pass, field, fieldMarkers, a.usePatchStrategy)
121127
a.checkFieldTags(pass, index, field)
122128

123129
if a.isFirstField == config.ConditionsFirstFieldWarn && index != 0 {
124130
pass.Reportf(field.Pos(), "Conditions field must be the first field in the struct")
125131
}
126132
}
127133

128-
func checkFieldMarkers(pass *analysis.Pass, field *ast.Field, fieldMarkers markers.MarkerSet) {
134+
func checkFieldMarkers(pass *analysis.Pass, field *ast.Field, fieldMarkers markers.MarkerSet, usePatchStrategy config.ConditionsUsePatchStrategy) {
129135
missingMarkers := []string{}
136+
additionalMarkers := []markers.Marker{}
130137

131138
if !fieldMarkers.HasWithValue(listTypeMap) {
132139
missingMarkers = append(missingMarkers, listTypeMap)
@@ -136,37 +143,101 @@ func checkFieldMarkers(pass *analysis.Pass, field *ast.Field, fieldMarkers marke
136143
missingMarkers = append(missingMarkers, listMapKeyType)
137144
}
138145

139-
if !fieldMarkers.HasWithValue(patchStrategy) {
140-
missingMarkers = append(missingMarkers, patchStrategy)
141-
}
146+
switch usePatchStrategy {
147+
case config.ConditionsUsePatchStrategySuggestFix, config.ConditionsUsePatchStrategyWarn:
148+
if !fieldMarkers.HasWithValue(patchStrategyMerge) {
149+
missingMarkers = append(missingMarkers, patchStrategyMerge)
150+
}
142151

143-
if !fieldMarkers.HasWithValue(patchMergeKeyType) {
144-
missingMarkers = append(missingMarkers, patchMergeKeyType)
152+
if !fieldMarkers.HasWithValue(patchMergeKeyType) {
153+
missingMarkers = append(missingMarkers, patchMergeKeyType)
154+
}
155+
case config.ConditionsUsePatchStrategyIgnore:
156+
// If it's there, we don't care.
157+
case config.ConditionsUsePatchStrategyForbid:
158+
if fieldMarkers.HasWithValue(patchStrategyMerge) {
159+
additionalMarkers = append(additionalMarkers, fieldMarkers[patchStrategy]...)
160+
}
161+
162+
if fieldMarkers.HasWithValue(patchMergeKeyType) {
163+
additionalMarkers = append(additionalMarkers, fieldMarkers[patchMergeKey]...)
164+
}
165+
default:
166+
panic("unexpected usePatchStrategy value")
145167
}
146168

147169
if !fieldMarkers.Has(optional) {
148170
missingMarkers = append(missingMarkers, optional)
149171
}
150172

151173
if len(missingMarkers) != 0 {
152-
pass.Report(analysis.Diagnostic{
153-
Pos: field.Pos(),
154-
End: field.End(),
155-
Message: "Conditions field is missing the following markers: " + strings.Join(missingMarkers, ", "),
156-
SuggestedFixes: []analysis.SuggestedFix{
157-
{
158-
Message: "Add missing markers",
159-
TextEdits: []analysis.TextEdit{
160-
{
161-
Pos: field.Pos(),
162-
End: token.NoPos,
163-
NewText: getNewMarkers(missingMarkers),
164-
},
174+
reportMissingMarkers(pass, field, missingMarkers, usePatchStrategy)
175+
}
176+
177+
if len(additionalMarkers) != 0 {
178+
reportAdditionalMarkers(pass, field, additionalMarkers)
179+
}
180+
}
181+
182+
func reportMissingMarkers(pass *analysis.Pass, field *ast.Field, missingMarkers []string, usePatchStrategy config.ConditionsUsePatchStrategy) {
183+
suggestedFixes := []analysis.SuggestedFix{}
184+
185+
// If patch strategy is warn, and the only markers in the list are patchStrategy and patchMergeKeyType, we don't need to suggest a fix.
186+
if usePatchStrategy != config.ConditionsUsePatchStrategyWarn || slices.ContainsFunc[[]string, string](missingMarkers, func(marker string) bool {
187+
switch marker {
188+
case patchStrategyMerge, patchMergeKeyType:
189+
return false
190+
default:
191+
return true
192+
}
193+
}) {
194+
suggestedFixes = []analysis.SuggestedFix{
195+
{
196+
Message: "Add missing markers",
197+
TextEdits: []analysis.TextEdit{
198+
{
199+
Pos: field.Pos(),
200+
End: token.NoPos,
201+
NewText: getNewMarkers(missingMarkers),
165202
},
166203
},
167204
},
205+
}
206+
}
207+
208+
pass.Report(analysis.Diagnostic{
209+
Pos: field.Pos(),
210+
End: field.End(),
211+
Message: "Conditions field is missing the following markers: " + strings.Join(missingMarkers, ", "),
212+
SuggestedFixes: suggestedFixes,
213+
})
214+
}
215+
216+
func reportAdditionalMarkers(pass *analysis.Pass, field *ast.Field, additionalMarkers []markers.Marker) {
217+
suggestedFixes := []analysis.SuggestedFix{}
218+
additionalMarkerValues := []string{}
219+
220+
for _, marker := range additionalMarkers {
221+
additionalMarkerValues = append(additionalMarkerValues, marker.String())
222+
223+
suggestedFixes = append(suggestedFixes, analysis.SuggestedFix{
224+
Message: "Remove additional marker",
225+
TextEdits: []analysis.TextEdit{
226+
{
227+
Pos: marker.Pos,
228+
End: marker.End + 1, // Add 1 to position to include the new line
229+
NewText: nil,
230+
},
231+
},
168232
})
169233
}
234+
235+
pass.Report(analysis.Diagnostic{
236+
Pos: field.Pos(),
237+
End: field.End(),
238+
Message: "Conditions field has the following additional markers: " + strings.Join(additionalMarkerValues, ", "),
239+
SuggestedFixes: suggestedFixes,
240+
})
170241
}
171242

172243
func getNewMarkers(missingMarkers []string) []byte {
@@ -181,7 +252,7 @@ func getNewMarkers(missingMarkers []string) []byte {
181252

182253
func (a *analyzer) checkFieldTags(pass *analysis.Pass, index int, field *ast.Field) {
183254
if field.Tag == nil {
184-
expectedTag := getExpectedTag(a.useProtobuf, a.isFirstField, index)
255+
expectedTag := getExpectedTag(a.usePatchStrategy, a.useProtobuf, a.isFirstField, index)
185256

186257
pass.Report(analysis.Diagnostic{
187258
Pos: field.Pos(),
@@ -204,9 +275,9 @@ func (a *analyzer) checkFieldTags(pass *analysis.Pass, index int, field *ast.Fie
204275
return
205276
}
206277

207-
asExpected, shouldFix := tagIsAsExpected(field.Tag.Value, a.useProtobuf, a.isFirstField, index)
278+
asExpected, shouldFix := tagIsAsExpected(field.Tag.Value, a.usePatchStrategy, a.useProtobuf, a.isFirstField, index)
208279
if !asExpected {
209-
expectedTag := getExpectedTag(a.useProtobuf, a.isFirstField, index)
280+
expectedTag := getExpectedTag(a.usePatchStrategy, a.useProtobuf, a.isFirstField, index)
210281

211282
if !shouldFix {
212283
pass.Reportf(field.Tag.ValuePos, "Conditions field has incorrect tags, should be: %s", expectedTag)
@@ -232,30 +303,65 @@ func (a *analyzer) checkFieldTags(pass *analysis.Pass, index int, field *ast.Fie
232303
}
233304
}
234305

235-
func getExpectedTag(useProtobuf config.ConditionsUseProtobuf, isFirstField config.ConditionsFirstField, index int) string {
306+
func getExpectedTag(usePatchStrategy config.ConditionsUsePatchStrategy, useProtobuf config.ConditionsUseProtobuf, isFirstField config.ConditionsFirstField, index int) string {
307+
expectedTag := fmt.Sprintf("`%s", expectedJSONTag)
308+
309+
if usePatchStrategy == config.ConditionsUsePatchStrategySuggestFix || usePatchStrategy == config.ConditionsUsePatchStrategyWarn {
310+
expectedTag += fmt.Sprintf(" %s", expectedPatchTag)
311+
}
312+
236313
if useProtobuf == config.ConditionsUseProtobufSuggestFix || useProtobuf == config.ConditionsUseProtobufWarn {
237-
i := 1
238-
if isFirstField == config.ConditionsFirstFieldIgnore {
239-
i = index + 1
240-
}
314+
expectedTag += fmt.Sprintf(" %s", getExpectedProtobufTag(isFirstField, index))
315+
}
316+
317+
expectedTag += "`"
318+
319+
return expectedTag
320+
}
241321

242-
return fmt.Sprintf(expectedTagWithProtobufFmt, i)
322+
func getExpectedProtobufTag(isFirstField config.ConditionsFirstField, index int) string {
323+
i := 1
324+
if isFirstField == config.ConditionsFirstFieldIgnore {
325+
i = index + 1
243326
}
244327

245-
return expectedTagWithoutProtobuf
328+
return fmt.Sprintf(expectedProtobufTag, i)
246329
}
247330

248-
func tagIsAsExpected(tag string, useProtobuf config.ConditionsUseProtobuf, isFirstField config.ConditionsFirstField, index int) (bool, bool) {
331+
func tagIsAsExpected(tag string, usePatchStrategy config.ConditionsUsePatchStrategy, useProtobuf config.ConditionsUseProtobuf, isFirstField config.ConditionsFirstField, index int) (bool, bool) {
332+
var tagIncorrect, shouldSuggestFix bool
333+
334+
switch usePatchStrategy {
335+
case config.ConditionsUsePatchStrategySuggestFix:
336+
tagIncorrect = tagIncorrect || !strings.Contains(tag, expectedPatchTag)
337+
shouldSuggestFix = true
338+
case config.ConditionsUsePatchStrategyWarn:
339+
tagIncorrect = tagIncorrect || !strings.Contains(tag, expectedPatchTag)
340+
case config.ConditionsUsePatchStrategyIgnore:
341+
// If it's there, we don't care.
342+
case config.ConditionsUsePatchStrategyForbid:
343+
tagIncorrect = tagIncorrect || strings.Contains(tag, expectedPatchTag)
344+
shouldSuggestFix = true
345+
default:
346+
panic("unexpected usePatchStrategy value")
347+
}
348+
249349
switch useProtobuf {
250350
case config.ConditionsUseProtobufSuggestFix:
251-
return tag == getExpectedTag(config.ConditionsUseProtobufSuggestFix, isFirstField, index), true
351+
tagIncorrect = tagIncorrect || !strings.Contains(tag, getExpectedProtobufTag(isFirstField, index))
352+
shouldSuggestFix = true
252353
case config.ConditionsUseProtobufWarn:
253-
return tag == getExpectedTag(config.ConditionsUseProtobufWarn, isFirstField, index), false
354+
tagIncorrect = tagIncorrect || !strings.Contains(tag, getExpectedProtobufTag(isFirstField, index))
254355
case config.ConditionsUseProtobufIgnore:
255-
return tag == getExpectedTag(config.ConditionsUseProtobufIgnore, isFirstField, index) || tag == getExpectedTag(config.ConditionsUseProtobufSuggestFix, isFirstField, index), true
356+
// If it's there, we don't care.
357+
case config.ConditionsUseProtobufForbid:
358+
tagIncorrect = tagIncorrect || strings.Contains(tag, getExpectedProtobufTag(isFirstField, index))
359+
shouldSuggestFix = true
256360
default:
257361
panic("unexpected useProtobuf value")
258362
}
363+
364+
return !tagIncorrect, shouldSuggestFix
259365
}
260366

261367
func fieldIsCalledConditions(field *ast.Field) bool {
@@ -309,4 +415,8 @@ func defaultConfig(cfg *config.ConditionsConfig) {
309415
if cfg.UseProtobuf == "" {
310416
cfg.UseProtobuf = config.ConditionsUseProtobufSuggestFix
311417
}
418+
419+
if cfg.UsePatchStrategy == "" {
420+
cfg.UsePatchStrategy = config.ConditionsUsePatchStrategySuggestFix
421+
}
312422
}

‎pkg/analysis/conditions/analyzer_test.go

+45
Original file line numberDiff line numberDiff line change
@@ -48,3 +48,48 @@ func TestIgnoreProtobuf(t *testing.T) {
4848

4949
analysistest.RunWithSuggestedFixes(t, testdata, a, "c")
5050
}
51+
52+
func TestForbidProtobuf(t *testing.T) {
53+
testdata := analysistest.TestData()
54+
55+
a, err := conditions.Initializer().Init(config.LintersConfig{
56+
Conditions: config.ConditionsConfig{
57+
UseProtobuf: config.ConditionsUseProtobufForbid,
58+
},
59+
})
60+
if err != nil {
61+
t.Fatal(err)
62+
}
63+
64+
analysistest.RunWithSuggestedFixes(t, testdata, a, "d")
65+
}
66+
67+
func TestIgnorePatchStrategy(t *testing.T) {
68+
testdata := analysistest.TestData()
69+
70+
a, err := conditions.Initializer().Init(config.LintersConfig{
71+
Conditions: config.ConditionsConfig{
72+
UsePatchStrategy: config.ConditionsUsePatchStrategyIgnore,
73+
},
74+
})
75+
if err != nil {
76+
t.Fatal(err)
77+
}
78+
79+
analysistest.RunWithSuggestedFixes(t, testdata, a, "e")
80+
}
81+
82+
func TestForbidPatchStrategy(t *testing.T) {
83+
testdata := analysistest.TestData()
84+
85+
a, err := conditions.Initializer().Init(config.LintersConfig{
86+
Conditions: config.ConditionsConfig{
87+
UsePatchStrategy: config.ConditionsUsePatchStrategyForbid,
88+
},
89+
})
90+
if err != nil {
91+
t.Fatal(err)
92+
}
93+
94+
analysistest.RunWithSuggestedFixes(t, testdata, a, "f")
95+
}

0 commit comments

Comments
 (0)