Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 1 addition & 29 deletions pkg/analysis/helpers/inspector/inspector.go
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ func (i *inspector) shouldProcessField(stack []ast.Node, skipListTypes bool) boo
return false
}

if skipListTypes && isItemsType(structType) {
if skipListTypes && utils.IsKubernetesListType(structType, "") {
// Skip list types if requested.
return false
}
Expand Down Expand Up @@ -168,34 +168,6 @@ func (i *inspector) InspectTypeSpec(inspectTypeSpec func(typeSpec *ast.TypeSpec,
})
}

func isItemsType(structType *ast.StructType) bool {
// An items type is a struct with TypeMeta, ListMeta and Items fields.
if len(structType.Fields.List) != 3 {
return false
}

// Check if the first field is TypeMeta.
// This should be a selector (e.g. metav1.TypeMeta)
// Check the TypeMeta part as the package name may vary.
if typeMeta, ok := structType.Fields.List[0].Type.(*ast.SelectorExpr); !ok || typeMeta.Sel.Name != "TypeMeta" {
return false
}

// Check if the second field is ListMeta.
if listMeta, ok := structType.Fields.List[1].Type.(*ast.SelectorExpr); !ok || listMeta.Sel.Name != "ListMeta" {
return false
}

// Check if the third field is Items.
// It should be an array, and be called Items.
itemsField := structType.Fields.List[2]
if _, ok := itemsField.Type.(*ast.ArrayType); !ok || len(itemsField.Names) == 0 || itemsField.Names[0].Name != "Items" {
return false
}

return true
}

func isSchemalessType(markerSet markers.MarkerSet) bool {
// Check if the field is marked as schemaless.
schemalessMarker := markerSet.Get(markersconsts.KubebuilderSchemaLessMarker)
Expand Down
62 changes: 35 additions & 27 deletions pkg/analysis/statussubresource/analyzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
kalerrors "sigs.k8s.io/kube-api-linter/pkg/analysis/errors"
"sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/extractjsontags"
markershelper "sigs.k8s.io/kube-api-linter/pkg/analysis/helpers/markers"
"sigs.k8s.io/kube-api-linter/pkg/analysis/utils"
"sigs.k8s.io/kube-api-linter/pkg/markers"
)

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

// Skip Kubernetes List types as they follow a different pattern
// and don't use the status subresource
if utils.IsKubernetesListType(sTyp, name) {
return
}

hasStatusSubresourceMarker := structMarkers.Has(markers.KubebuilderStatusSubresourceMarker)
hasStatusField := hasStatusField(sTyp, jsonTags)

switch {
case (hasStatusSubresourceMarker && hasStatusField), (!hasStatusSubresourceMarker && !hasStatusField):
// acceptable state
case hasStatusSubresourceMarker && !hasStatusField:
// Might be able to have some suggested fixes here, but it is likely much more complex
// so for now leave it with a descriptive failure message.
// Both present or both absent is acceptable
if hasStatusSubresourceMarker == hasStatusField {
return
}

// Marker present but no status field
if hasStatusSubresourceMarker {
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)
case !hasStatusSubresourceMarker && hasStatusField:
// In this case we can suggest the autofix to add the status subresource marker
pass.Report(analysis.Diagnostic{
Pos: sTyp.Pos(),
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),
SuggestedFixes: []analysis.SuggestedFix{
{
Message: "should add the kubebuilder:subresource:status marker",
TextEdits: []analysis.TextEdit{
// go one line above the struct and add the marker
{
// sTyp.Pos() is the beginning of the 'struct' keyword. Subtract
// the length of the struct name + 7 (2 for spaces surrounding type name, 4 for the 'type' keyword,
// and 1 for the newline) to position at the end of the line above the struct
// definition.
Pos: sTyp.Pos() - token.Pos(len(name)+7),
// prefix with a newline to ensure we aren't appending to a previous comment
NewText: []byte("\n// +kubebuilder:subresource:status"),
},
return
}

// Status field present but no marker - suggest autofix
pass.Report(analysis.Diagnostic{
Pos: sTyp.Pos(),
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),
SuggestedFixes: []analysis.SuggestedFix{
{
Message: "should add the kubebuilder:subresource:status marker",
TextEdits: []analysis.TextEdit{
{
// sTyp.Pos() is the beginning of the 'struct' keyword. Subtract
// the length of the struct name + 7 (2 for spaces surrounding type name, 4 for the 'type' keyword,
// and 1 for the newline) to position at the end of the line above the struct
// definition.
Pos: sTyp.Pos() - token.Pos(len(name)+7),
// prefix with a newline to ensure we aren't appending to a previous comment
NewText: []byte("\n// +kubebuilder:subresource:status"),
Comment on lines -103 to +136
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactoring this keeps the cyclo <=10.

},
},
},
})
}
},
})
}

func hasStatusField(sTyp *ast.StructType, jsonTags extractjsontags.StructFieldTags) bool {
Expand Down
119 changes: 117 additions & 2 deletions pkg/analysis/statussubresource/testdata/src/a/a.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,124 @@ type FooBarStatus struct {
// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
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"
Spec FooBarBazSpec `json:"spec"`
Spec FooBarBazSpec `json:"spec"`
}

type FooBarBazSpec struct {
Name string `json:"name"`
Name string `json:"name"`
}

// Test that List types are skipped (issue #53)
// This is a Kubernetes List type with TypeMeta, ListMeta, and Items
// Even with space between marker and type definition, it should be skipped

// +kubebuilder:object:root=true
type ClusterList struct {
TypeMeta `json:",inline"`
ListMeta `json:"metadata,omitempty"`
Items []Cluster `json:"items"`
}

type TypeMeta struct {
Kind string `json:"kind,omitempty"`
APIVersion string `json:"apiVersion,omitempty"`
}

type ListMeta struct {
ResourceVersion string `json:"resourceVersion,omitempty"`
}

type Cluster struct {
Name string `json:"name"`
}

// Test List type with qualified types (metav1.TypeMeta, metav1.ListMeta)
// +kubebuilder:object:root=true
type PodList struct {
metav1TypeMeta `json:",inline"`
metav1ListMeta `json:"metadata,omitempty"`
Items []Pod `json:"items"`
}

type metav1TypeMeta struct {
Kind string `json:"kind,omitempty"`
APIVersion string `json:"apiVersion,omitempty"`
}

type metav1ListMeta struct {
ResourceVersion string `json:"resourceVersion,omitempty"`
}

type Pod struct {
Name string `json:"name"`
}

// Test that non-List types are not skipped even if they have 3 fields
// This should trigger the linter because it has a status field but no marker
// +kubebuilder:object:root=true
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"
Spec NotAListSpec `json:"spec"`
Status NotAListStatus `json:"status"`
Extra string `json:"extra"`
}

type NotAListSpec struct {
Name string `json:"name"`
}

type NotAListStatus struct {
Ready bool `json:"ready"`
}

// Test that types ending with "List" but not matching the pattern are not skipped
// This has 4 fields instead of 3, so it should not be treated as a List type
// +kubebuilder:object:root=true
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"
TypeMeta `json:",inline"`
ListMeta `json:"metadata,omitempty"`
Items []string `json:"items"`
Status BadListStatus `json:"status"`
}

type BadListStatus struct {
Count int `json:"count"`
}

// Test that types ending with "List" but missing Items field are not skipped
// +kubebuilder:object:root=true
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"
TypeMeta `json:",inline"`
ListMeta `json:"metadata,omitempty"`
Status IncompleteListStatus `json:"status"`
}

type IncompleteListStatus struct {
Ready bool `json:"ready"`
}

// Test that types ending with "List" but with non-slice Items field are not skipped
// Items must be a slice type to be considered a valid List type
// +kubebuilder:object:root=true
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"
TypeMeta `json:",inline"`
ListMeta `json:"metadata,omitempty"`
Items string `json:"items"`
Status NonSliceStatus `json:"status"`
}

type NonSliceStatus struct {
Count int `json:"count"`
}

// Test that List types with fields in non-standard order are still recognized
// Fields can be in any order as long as they have TypeMeta, ListMeta, and Items
// +kubebuilder:object:root=true
type ReorderedFieldsList struct {
Items []ReorderedItem `json:"items"`
TypeMeta `json:",inline"`
ListMeta `json:"metadata,omitempty"`
}

type ReorderedItem struct {
Name string `json:"name"`
}
123 changes: 121 additions & 2 deletions pkg/analysis/statussubresource/testdata/src/a/a.go.golden
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,128 @@ type FooBarStatus struct {
// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
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"
Spec FooBarBazSpec `json:"spec"`
Spec FooBarBazSpec `json:"spec"`
}

type FooBarBazSpec struct {
Name string `json:"name"`
Name string `json:"name"`
}

// Test that List types are skipped (issue #53)
// This is a Kubernetes List type with TypeMeta, ListMeta, and Items
// Even with space between marker and type definition, it should be skipped

// +kubebuilder:object:root=true
type ClusterList struct {
TypeMeta `json:",inline"`
ListMeta `json:"metadata,omitempty"`
Items []Cluster `json:"items"`
}

type TypeMeta struct {
Kind string `json:"kind,omitempty"`
APIVersion string `json:"apiVersion,omitempty"`
}

type ListMeta struct {
ResourceVersion string `json:"resourceVersion,omitempty"`
}

type Cluster struct {
Name string `json:"name"`
}

// Test List type with qualified types (metav1.TypeMeta, metav1.ListMeta)
// +kubebuilder:object:root=true
type PodList struct {
metav1TypeMeta `json:",inline"`
metav1ListMeta `json:"metadata,omitempty"`
Items []Pod `json:"items"`
}

type metav1TypeMeta struct {
Kind string `json:"kind,omitempty"`
APIVersion string `json:"apiVersion,omitempty"`
}

type metav1ListMeta struct {
ResourceVersion string `json:"resourceVersion,omitempty"`
}

type Pod struct {
Name string `json:"name"`
}

// Test that non-List types are not skipped even if they have 3 fields
// This should trigger the linter because it has a status field but no marker
// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
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"
Spec NotAListSpec `json:"spec"`
Status NotAListStatus `json:"status"`
Extra string `json:"extra"`
}

type NotAListSpec struct {
Name string `json:"name"`
}

type NotAListStatus struct {
Ready bool `json:"ready"`
}

// Test that types ending with "List" but not matching the pattern are not skipped
// This has 4 fields instead of 3, so it should not be treated as a List type
// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
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"
TypeMeta `json:",inline"`
ListMeta `json:"metadata,omitempty"`
Items []string `json:"items"`
Status BadListStatus `json:"status"`
}

type BadListStatus struct {
Count int `json:"count"`
}

// Test that types ending with "List" but missing Items field are not skipped
// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
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"
TypeMeta `json:",inline"`
ListMeta `json:"metadata,omitempty"`
Status IncompleteListStatus `json:"status"`
}

type IncompleteListStatus struct {
Ready bool `json:"ready"`
}

// Test that types ending with "List" but with non-slice Items field are not skipped
// Items must be a slice type to be considered a valid List type
// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
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"
TypeMeta `json:",inline"`
ListMeta `json:"metadata,omitempty"`
Items string `json:"items"`
Status NonSliceStatus `json:"status"`
}

type NonSliceStatus struct {
Count int `json:"count"`
}

// Test that List types with fields in non-standard order are still recognized
// Fields can be in any order as long as they have TypeMeta, ListMeta, and Items
// +kubebuilder:object:root=true
type ReorderedFieldsList struct {
Items []ReorderedItem `json:"items"`
TypeMeta `json:",inline"`
ListMeta `json:"metadata,omitempty"`
}

type ReorderedItem struct {
Name string `json:"name"`
}
Loading