Skip to content

Commit a9a8d5c

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #177 from saschagrunert/fix-issue-53-marker-spacing
Skip statussubresource linting for Kubernetes List types
2 parents 584c41b + 799c28e commit a9a8d5c

File tree

5 files changed

+355
-60
lines changed

5 files changed

+355
-60
lines changed

‎pkg/analysis/helpers/inspector/inspector.go‎

Lines changed: 1 addition & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ func (i *inspector) shouldProcessField(stack []ast.Node, skipListTypes bool) boo
117117
return false
118118
}
119119

120-
if skipListTypes && isItemsType(structType) {
120+
if skipListTypes && utils.IsKubernetesListType(structType, "") {
121121
// Skip list types if requested.
122122
return false
123123
}
@@ -168,34 +168,6 @@ func (i *inspector) InspectTypeSpec(inspectTypeSpec func(typeSpec *ast.TypeSpec,
168168
})
169169
}
170170

171-
func isItemsType(structType *ast.StructType) bool {
172-
// An items type is a struct with TypeMeta, ListMeta and Items fields.
173-
if len(structType.Fields.List) != 3 {
174-
return false
175-
}
176-
177-
// Check if the first field is TypeMeta.
178-
// This should be a selector (e.g. metav1.TypeMeta)
179-
// Check the TypeMeta part as the package name may vary.
180-
if typeMeta, ok := structType.Fields.List[0].Type.(*ast.SelectorExpr); !ok || typeMeta.Sel.Name != "TypeMeta" {
181-
return false
182-
}
183-
184-
// Check if the second field is ListMeta.
185-
if listMeta, ok := structType.Fields.List[1].Type.(*ast.SelectorExpr); !ok || listMeta.Sel.Name != "ListMeta" {
186-
return false
187-
}
188-
189-
// Check if the third field is Items.
190-
// It should be an array, and be called Items.
191-
itemsField := structType.Fields.List[2]
192-
if _, ok := itemsField.Type.(*ast.ArrayType); !ok || len(itemsField.Names) == 0 || itemsField.Names[0].Name != "Items" {
193-
return false
194-
}
195-
196-
return true
197-
}
198-
199171
func isSchemalessType(markerSet markers.MarkerSet) bool {
200172
// Check if the field is marked as schemaless.
201173
schemalessMarker := markerSet.Get(markersconsts.KubebuilderSchemaLessMarker)

‎pkg/analysis/statussubresource/analyzer.go‎

Lines changed: 35 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@ import (
2626
kalerrors "sigs.k8s.io/kube-api-linter/pkg/analysis/errors"
2727
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags"
2828
markershelper "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/markers"
29+
"sigs.k8s.io/kube-api-linter/pkg/analysis/utils"
2930
"sigs.k8s.io/kube-api-linter/pkg/markers"
3031
)
3132

@@ -97,40 +98,47 @@ func checkStruct(pass *analysis.Pass, sTyp *ast.StructType, name string, structM
9798
return
9899
}
99100

101+
// Skip Kubernetes List types as they follow a different pattern
102+
// and don't use the status subresource
103+
if utils.IsKubernetesListType(sTyp, name) {
104+
return
105+
}
106+
100107
hasStatusSubresourceMarker := structMarkers.Has(markers.KubebuilderStatusSubresourceMarker)
101108
hasStatusField := hasStatusField(sTyp, jsonTags)
102109

103-
switch {
104-
case (hasStatusSubresourceMarker && hasStatusField), (!hasStatusSubresourceMarker && !hasStatusField):
105-
// acceptable state
106-
case hasStatusSubresourceMarker && !hasStatusField:
107-
// Might be able to have some suggested fixes here, but it is likely much more complex
108-
// so for now leave it with a descriptive failure message.
110+
// Both present or both absent is acceptable
111+
if hasStatusSubresourceMarker == hasStatusField {
112+
return
113+
}
114+
115+
// Marker present but no status field
116+
if hasStatusSubresourceMarker {
109117
pass.Reportf(sTyp.Pos(), "root object type %q is marked to enable the status subresource with marker %q but has no status field", name, markers.KubebuilderStatusSubresourceMarker)
110-
case !hasStatusSubresourceMarker && hasStatusField:
111-
// In this case we can suggest the autofix to add the status subresource marker
112-
pass.Report(analysis.Diagnostic{
113-
Pos: sTyp.Pos(),
114-
Message: fmt.Sprintf("root object type %q has a status field but does not have the marker %q to enable the status subresource", name, markers.KubebuilderStatusSubresourceMarker),
115-
SuggestedFixes: []analysis.SuggestedFix{
116-
{
117-
Message: "should add the kubebuilder:subresource:status marker",
118-
TextEdits: []analysis.TextEdit{
119-
// go one line above the struct and add the marker
120-
{
121-
// sTyp.Pos() is the beginning of the 'struct' keyword. Subtract
122-
// the length of the struct name + 7 (2 for spaces surrounding type name, 4 for the 'type' keyword,
123-
// and 1 for the newline) to position at the end of the line above the struct
124-
// definition.
125-
Pos: sTyp.Pos() - token.Pos(len(name)+7),
126-
// prefix with a newline to ensure we aren't appending to a previous comment
127-
NewText: []byte("\n// +kubebuilder:subresource:status"),
128-
},
118+
return
119+
}
120+
121+
// Status field present but no marker - suggest autofix
122+
pass.Report(analysis.Diagnostic{
123+
Pos: sTyp.Pos(),
124+
Message: fmt.Sprintf("root object type %q has a status field but does not have the marker %q to enable the status subresource", name, markers.KubebuilderStatusSubresourceMarker),
125+
SuggestedFixes: []analysis.SuggestedFix{
126+
{
127+
Message: "should add the kubebuilder:subresource:status marker",
128+
TextEdits: []analysis.TextEdit{
129+
{
130+
// sTyp.Pos() is the beginning of the 'struct' keyword. Subtract
131+
// the length of the struct name + 7 (2 for spaces surrounding type name, 4 for the 'type' keyword,
132+
// and 1 for the newline) to position at the end of the line above the struct
133+
// definition.
134+
Pos: sTyp.Pos() - token.Pos(len(name)+7),
135+
// prefix with a newline to ensure we aren't appending to a previous comment
136+
NewText: []byte("\n// +kubebuilder:subresource:status"),
129137
},
130138
},
131139
},
132-
})
133-
}
140+
},
141+
})
134142
}
135143

136144
func hasStatusField(sTyp *ast.StructType, jsonTags extractjsontags.StructFieldTags) bool {

‎pkg/analysis/statussubresource/testdata/src/a/a.go‎

Lines changed: 117 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,9 +52,124 @@ type FooBarStatus struct {
5252
// +kubebuilder:object:root=true
5353
// +kubebuilder:subresource:status
5454
type FooBarBaz struct { // want "root object type \"FooBarBaz\" is marked to enable the status subresource with marker \"kubebuilder:subresource:status\" but has no status field"
55-
Spec FooBarBazSpec `json:"spec"`
55+
Spec FooBarBazSpec `json:"spec"`
5656
}
5757

5858
type FooBarBazSpec struct {
59-
Name string `json:"name"`
59+
Name string `json:"name"`
60+
}
61+
62+
// Test that List types are skipped (issue #53)
63+
// This is a Kubernetes List type with TypeMeta, ListMeta, and Items
64+
// Even with space between marker and type definition, it should be skipped
65+
66+
// +kubebuilder:object:root=true
67+
type ClusterList struct {
68+
TypeMeta `json:",inline"`
69+
ListMeta `json:"metadata,omitempty"`
70+
Items []Cluster `json:"items"`
71+
}
72+
73+
type TypeMeta struct {
74+
Kind string `json:"kind,omitempty"`
75+
APIVersion string `json:"apiVersion,omitempty"`
76+
}
77+
78+
type ListMeta struct {
79+
ResourceVersion string `json:"resourceVersion,omitempty"`
80+
}
81+
82+
type Cluster struct {
83+
Name string `json:"name"`
84+
}
85+
86+
// Test List type with qualified types (metav1.TypeMeta, metav1.ListMeta)
87+
// +kubebuilder:object:root=true
88+
type PodList struct {
89+
metav1TypeMeta `json:",inline"`
90+
metav1ListMeta `json:"metadata,omitempty"`
91+
Items []Pod `json:"items"`
92+
}
93+
94+
type metav1TypeMeta struct {
95+
Kind string `json:"kind,omitempty"`
96+
APIVersion string `json:"apiVersion,omitempty"`
97+
}
98+
99+
type metav1ListMeta struct {
100+
ResourceVersion string `json:"resourceVersion,omitempty"`
101+
}
102+
103+
type Pod struct {
104+
Name string `json:"name"`
105+
}
106+
107+
// Test that non-List types are not skipped even if they have 3 fields
108+
// This should trigger the linter because it has a status field but no marker
109+
// +kubebuilder:object:root=true
110+
type NotAList struct { // want "root object type \"NotAList\" has a status field but does not have the marker \"kubebuilder:subresource:status\" to enable the status subresource"
111+
Spec NotAListSpec `json:"spec"`
112+
Status NotAListStatus `json:"status"`
113+
Extra string `json:"extra"`
114+
}
115+
116+
type NotAListSpec struct {
117+
Name string `json:"name"`
118+
}
119+
120+
type NotAListStatus struct {
121+
Ready bool `json:"ready"`
122+
}
123+
124+
// Test that types ending with "List" but not matching the pattern are not skipped
125+
// This has 4 fields instead of 3, so it should not be treated as a List type
126+
// +kubebuilder:object:root=true
127+
type BadList struct { // want "root object type \"BadList\" has a status field but does not have the marker \"kubebuilder:subresource:status\" to enable the status subresource"
128+
TypeMeta `json:",inline"`
129+
ListMeta `json:"metadata,omitempty"`
130+
Items []string `json:"items"`
131+
Status BadListStatus `json:"status"`
132+
}
133+
134+
type BadListStatus struct {
135+
Count int `json:"count"`
136+
}
137+
138+
// Test that types ending with "List" but missing Items field are not skipped
139+
// +kubebuilder:object:root=true
140+
type IncompleteList struct { // want "root object type \"IncompleteList\" has a status field but does not have the marker \"kubebuilder:subresource:status\" to enable the status subresource"
141+
TypeMeta `json:",inline"`
142+
ListMeta `json:"metadata,omitempty"`
143+
Status IncompleteListStatus `json:"status"`
144+
}
145+
146+
type IncompleteListStatus struct {
147+
Ready bool `json:"ready"`
148+
}
149+
150+
// Test that types ending with "List" but with non-slice Items field are not skipped
151+
// Items must be a slice type to be considered a valid List type
152+
// +kubebuilder:object:root=true
153+
type NonSliceItemsList struct { // want "root object type \"NonSliceItemsList\" has a status field but does not have the marker \"kubebuilder:subresource:status\" to enable the status subresource"
154+
TypeMeta `json:",inline"`
155+
ListMeta `json:"metadata,omitempty"`
156+
Items string `json:"items"`
157+
Status NonSliceStatus `json:"status"`
158+
}
159+
160+
type NonSliceStatus struct {
161+
Count int `json:"count"`
162+
}
163+
164+
// Test that List types with fields in non-standard order are still recognized
165+
// Fields can be in any order as long as they have TypeMeta, ListMeta, and Items
166+
// +kubebuilder:object:root=true
167+
type ReorderedFieldsList struct {
168+
Items []ReorderedItem `json:"items"`
169+
TypeMeta `json:",inline"`
170+
ListMeta `json:"metadata,omitempty"`
171+
}
172+
173+
type ReorderedItem struct {
174+
Name string `json:"name"`
60175
}

‎pkg/analysis/statussubresource/testdata/src/a/a.go.golden‎

Lines changed: 121 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,9 +53,128 @@ type FooBarStatus struct {
5353
// +kubebuilder:object:root=true
5454
// +kubebuilder:subresource:status
5555
type FooBarBaz struct { // want "root object type \"FooBarBaz\" is marked to enable the status subresource with marker \"kubebuilder:subresource:status\" but has no status field"
56-
Spec FooBarBazSpec `json:"spec"`
56+
Spec FooBarBazSpec `json:"spec"`
5757
}
5858

5959
type FooBarBazSpec struct {
60-
Name string `json:"name"`
60+
Name string `json:"name"`
61+
}
62+
63+
// Test that List types are skipped (issue #53)
64+
// This is a Kubernetes List type with TypeMeta, ListMeta, and Items
65+
// Even with space between marker and type definition, it should be skipped
66+
67+
// +kubebuilder:object:root=true
68+
type ClusterList struct {
69+
TypeMeta `json:",inline"`
70+
ListMeta `json:"metadata,omitempty"`
71+
Items []Cluster `json:"items"`
72+
}
73+
74+
type TypeMeta struct {
75+
Kind string `json:"kind,omitempty"`
76+
APIVersion string `json:"apiVersion,omitempty"`
77+
}
78+
79+
type ListMeta struct {
80+
ResourceVersion string `json:"resourceVersion,omitempty"`
81+
}
82+
83+
type Cluster struct {
84+
Name string `json:"name"`
85+
}
86+
87+
// Test List type with qualified types (metav1.TypeMeta, metav1.ListMeta)
88+
// +kubebuilder:object:root=true
89+
type PodList struct {
90+
metav1TypeMeta `json:",inline"`
91+
metav1ListMeta `json:"metadata,omitempty"`
92+
Items []Pod `json:"items"`
93+
}
94+
95+
type metav1TypeMeta struct {
96+
Kind string `json:"kind,omitempty"`
97+
APIVersion string `json:"apiVersion,omitempty"`
98+
}
99+
100+
type metav1ListMeta struct {
101+
ResourceVersion string `json:"resourceVersion,omitempty"`
102+
}
103+
104+
type Pod struct {
105+
Name string `json:"name"`
106+
}
107+
108+
// Test that non-List types are not skipped even if they have 3 fields
109+
// This should trigger the linter because it has a status field but no marker
110+
// +kubebuilder:object:root=true
111+
// +kubebuilder:subresource:status
112+
type NotAList struct { // want "root object type \"NotAList\" has a status field but does not have the marker \"kubebuilder:subresource:status\" to enable the status subresource"
113+
Spec NotAListSpec `json:"spec"`
114+
Status NotAListStatus `json:"status"`
115+
Extra string `json:"extra"`
116+
}
117+
118+
type NotAListSpec struct {
119+
Name string `json:"name"`
120+
}
121+
122+
type NotAListStatus struct {
123+
Ready bool `json:"ready"`
124+
}
125+
126+
// Test that types ending with "List" but not matching the pattern are not skipped
127+
// This has 4 fields instead of 3, so it should not be treated as a List type
128+
// +kubebuilder:object:root=true
129+
// +kubebuilder:subresource:status
130+
type BadList struct { // want "root object type \"BadList\" has a status field but does not have the marker \"kubebuilder:subresource:status\" to enable the status subresource"
131+
TypeMeta `json:",inline"`
132+
ListMeta `json:"metadata,omitempty"`
133+
Items []string `json:"items"`
134+
Status BadListStatus `json:"status"`
135+
}
136+
137+
type BadListStatus struct {
138+
Count int `json:"count"`
139+
}
140+
141+
// Test that types ending with "List" but missing Items field are not skipped
142+
// +kubebuilder:object:root=true
143+
// +kubebuilder:subresource:status
144+
type IncompleteList struct { // want "root object type \"IncompleteList\" has a status field but does not have the marker \"kubebuilder:subresource:status\" to enable the status subresource"
145+
TypeMeta `json:",inline"`
146+
ListMeta `json:"metadata,omitempty"`
147+
Status IncompleteListStatus `json:"status"`
148+
}
149+
150+
type IncompleteListStatus struct {
151+
Ready bool `json:"ready"`
152+
}
153+
154+
// Test that types ending with "List" but with non-slice Items field are not skipped
155+
// Items must be a slice type to be considered a valid List type
156+
// +kubebuilder:object:root=true
157+
// +kubebuilder:subresource:status
158+
type NonSliceItemsList struct { // want "root object type \"NonSliceItemsList\" has a status field but does not have the marker \"kubebuilder:subresource:status\" to enable the status subresource"
159+
TypeMeta `json:",inline"`
160+
ListMeta `json:"metadata,omitempty"`
161+
Items string `json:"items"`
162+
Status NonSliceStatus `json:"status"`
163+
}
164+
165+
type NonSliceStatus struct {
166+
Count int `json:"count"`
167+
}
168+
169+
// Test that List types with fields in non-standard order are still recognized
170+
// Fields can be in any order as long as they have TypeMeta, ListMeta, and Items
171+
// +kubebuilder:object:root=true
172+
type ReorderedFieldsList struct {
173+
Items []ReorderedItem `json:"items"`
174+
TypeMeta `json:",inline"`
175+
ListMeta `json:"metadata,omitempty"`
176+
}
177+
178+
type ReorderedItem struct {
179+
Name string `json:"name"`
61180
}

0 commit comments

Comments
 (0)