add statussubresource lint check

[everettraven]

No description provided.

[JoelSpeed]

Raw inserts can be achieved by setting Pos to the sTyp.Pos(), and setting End to token.NoPos. I think this will achieve what you want here

[everettraven]

I'll play around with this a bit more and see if I can get something cleaner. The key thing here is that I want the insert to be on the line above the struct

[JoelSpeed]

The conditions linter I'm working on presently has an example of this same thing, and it's working as I'm expecting, that's how I knew to comment here. LMK if you can't get it working and I'll send you a snippet

[everettraven]

I couldn't get what you mentioned to work for my case. A snippet would be helpful, but setting:

{
    Pos: sTyp.Pos(),
    End: token.NoPos,
    ...
}

resulted in this:

// +kubebuilder:object:root:=true
type Baz 
// +kubebuilder:subresource:statusstruct {

I modified my original logic to be a bit more straightforward and explained it with a comment. With my existing logic I get the expected outcome of:

// +kubebuilder:object:root:=true
// +kubebuilder:subresource:status
type Baz struct {

[JoelSpeed]

Do these need to be exported?

[everettraven]

Probably not. Will unexport them

[everettraven]

unexported them in the latest push

[JoelSpeed]

So I was hacking on the maxLength implementation just before shutdown and came to a similar conclusion here, except, I concluded that I no longer needed TypeSpec in the filter, and got rid of extractTypeSpecMarkers.

My extractGenDeclMarkers looked like

func extractGenDeclMarkers(typ *ast.GenDecl, results *markers) {
	declMarkers := NewMarkerSet()

	if typ.Doc != nil {
		for _, comment := range typ.Doc.List {
			if marker := extractMarker(comment); marker.Value != "" {
				declMarkers.Insert(marker)
			}
		}
	}

	if len(typ.Specs) == 0 {
		return
	}

	tSpec, ok := typ.Specs[0].(*ast.TypeSpec)
	if !ok {
		return
	}

	results.insertTypeMarkers(tSpec, declMarkers)

	if sTyp, ok := tSpec.Type.(*ast.StructType); ok {
		results.insertStructMarkers(sTyp, declMarkers)
	}
}

So pretty similar conclusion, except I didn't iterate over a slice of specs. Is it actually possible for a GenDecl to represent more than one spec?

In my approach, I also didn't have mergeMarkerSets, that didn't seem necessary, any ideas why you needed it?

[everettraven]

My understanding, based on the information from golang/go#27477, is that you could define types in this format:

// GenDecl picks up comments here
type (
    // TypeSpec picks up comments here
    Foo struct {...}
    
    // TypeSpec here too
    Bar struct {...}
)

Maybe we don't care about the fact that things could be defined that way, but I tried to write the implementation in this PR such that it respects that someone is able to do this in Go. I'm not sure it is a correct assumption, but I assumed that any comments picked up when parsing GenDecl comments should probably also be added to the types themselves - I can try to do more research on this assumption to see how controller-tools would handle a scenario like this.

The mergeMarkerSets was necessary since I kept the ast.TypeSpec node type in the list to facilitate the merging of any markers defined at the various scopes. The merging is essentially making something like:

// +kubebuilder:validation:MaxLength:=10
type (
    // +kubebuilder:validation:Pattern:='^[0-9A-Za-z]*$'
    Foo string

    // +kubebuilder:validation:Pattern:='^[a-z]*$'
    Bar string
)

Equivalent to:

// +kubebuilder:validation:MaxLength:=10 
// +kubebuilder:validation:Pattern:='^[0-9A-Za-z]*$'
type Foo string

// +kubebuilder:validation:MaxLength:=10
// +kubebuilder:validation:Pattern:='^[a-z]*$'
type Bar string

[JoelSpeed]

I'm not sure it is a correct assumption, but I assumed that any comments picked up when parsing GenDecl comments should probably also be added to the types themselves - I can try to do more research on this assumption to see how controller-tools would handle a scenario like this.

I think I would likely disagree with that assumption, in this case I would expect the comments on the TypeSpec to apply, not the GenDecl comments.

I have also never seen this pattern used.

If you think this is reasonable to support, I would make sure we have tests for this, and we need to agree on the merging. For now, personally I would lean towards not supporting this type of declaration until/unless someone uses it (this is an early stage project, we don't have to support everything right now). A good smoke test would be to check if you can see this used in k/api at all.

[JoelSpeed]

Nit, could combine these right?

[everettraven]

Ah, yeah I could.

[JoelSpeed]

Yes, you have the test I wanted to see, awesome :D