🌱 Add comments for spec/status/items #11870

tsuzu · 2025-02-18T21:10:42Z

What this PR does / why we need it:

Add comments for spec/status/items fields

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
A part of #11238

Once this PR is merged, all comments in API resources would start with serialized names.

Task list: #11238 (comment)

/area documentation

k8s-ci-robot · 2025-02-18T21:10:51Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot · 2025-02-18T21:10:53Z

Hi @tsuzu. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

tsuzu · 2025-02-18T21:43:48Z

I've tried to run kal with commentstart enabled, and I've found some false positives.
cc @JoelSpeed

Non-API resources (can be skipped with path)

exp/runtime/hooks/api/v1alpha1/topologymutation_variable_types.go:30:2: commentstart: field Cluster is missing godoc comment (kal)
        Cluster           *ClusterBuiltins           `json:"cluster,omitempty"`
        ^
exp/runtime/hooks/api/v1alpha1/topologymutation_variable_types.go:31:2: commentstart: field ControlPlane is missing godoc comment (kal)
        ControlPlane      *ControlPlaneBuiltins      `json:"controlPlane,omitempty"`
        ^
exp/runtime/hooks/api/v1alpha1/topologymutation_variable_types.go:32:2: commentstart: field MachineDeployment is missing godoc comment (kal)
        MachineDeployment *MachineDeploymentBuiltins `json:"machineDeployment,omitempty"`
        ^
exp/runtime/hooks/api/v1alpha1/topologymutation_variable_types.go:33:2: commentstart: field MachinePool is missing godoc comment (kal)
        MachinePool       *MachinePoolBuiltins       `json:"machinePool,omitempty"`
        ^

cmd/clusterctl/api/v1alpha3/metadata_type.go:32:2: commentstart: godoc for field ReleaseSeries should start with 'releaseSeries ...' (kal)
        // +optional
        ^

Inline non-API resources(can ignore inline fields?)

bootstrap/kubeadm/api/v1beta1/kubeadm_types.go:289:4: commentstart: field Name is missing godoc comment (kal)
                        Name                  string            `json:"name,omitempty"`
                        ^
bootstrap/kubeadm/api/v1beta1/kubeadm_types.go:290:4: commentstart: field CRISocket is missing godoc comment (kal)
                        CRISocket             string            `json:"criSocket,omitempty"`
                        ^
bootstrap/kubeadm/api/v1beta1/kubeadm_types.go:291:4: commentstart: field Taints is missing godoc comment (kal)
                        Taints                []corev1.Taint    `json:"taints"`
                        ^
bootstrap/kubeadm/api/v1beta1/kubeadm_types.go:292:4: commentstart: field KubeletExtraArgs is missing godoc comment (kal)
                        KubeletExtraArgs      map[string]string `json:"kubeletExtraArgs,omitempty"`
                        ^
bootstrap/kubeadm/api/v1beta1/kubeadm_types.go:293:4: commentstart: field IgnorePreflightErrors is missing godoc comment (kal)
                        IgnorePreflightErrors []string          `json:"ignorePreflightErrors,omitempty"`
                        ^
bootstrap/kubeadm/api/v1beta1/kubeadm_types.go:294:4: commentstart: field ImagePullPolicy is missing godoc comment (kal)
                        ImagePullPolicy       string            `json:"imagePullPolicy,omitempty"`
                        ^
bootstrap/kubeadm/api/v1beta1/kubeadm_types.go:295:4: commentstart: field ImagePullSerial is missing godoc comment (kal)
                        ImagePullSerial       *bool             `json:"imagePullSerial,omitempty"`
                        ^
bootstrap/kubeadm/api/v1beta1/kubeadm_types.go:309:3: commentstart: field Name is missing godoc comment (kal)
                Name                  string            `json:"name,omitempty"`
                ^
bootstrap/kubeadm/api/v1beta1/kubeadm_types.go:310:3: commentstart: field CRISocket is missing godoc comment (kal)
                CRISocket             string            `json:"criSocket,omitempty"`
                ^
bootstrap/kubeadm/api/v1beta1/kubeadm_types.go:311:3: commentstart: field Taints is missing godoc comment (kal)
                Taints                []corev1.Taint    `json:"taints,omitempty"`
                ^
bootstrap/kubeadm/api/v1beta1/kubeadm_types.go:312:3: commentstart: field KubeletExtraArgs is missing godoc comment (kal)
                KubeletExtraArgs      map[string]string `json:"kubeletExtraArgs,omitempty"`
                ^
bootstrap/kubeadm/api/v1beta1/kubeadm_types.go:313:3: commentstart: field IgnorePreflightErrors is missing godoc comment (kal)
                IgnorePreflightErrors []string          `json:"ignorePreflightErrors,omitempty"`
                ^
bootstrap/kubeadm/api/v1beta1/kubeadm_types.go:314:3: commentstart: field ImagePullPolicy is missing godoc comment (kal)
                ImagePullPolicy       string            `json:"imagePullPolicy,omitempty"`
                ^
bootstrap/kubeadm/api/v1beta1/kubeadm_types.go:315:3: commentstart: field ImagePullSerial is missing godoc comment (kal)
                ImagePullSerial       *bool             `json:"imagePullSerial,omitempty"`
                ^

Omitted fields should be ignored (Ignore if json tag is -, prepared a PR JoelSpeed/kal#39 )

bootstrap/kubeadm/api/v1beta1/kubeadm_types.go:696:2: commentstart: field ID is missing godoc comment (kal)
        ID     string `json:"-"`
        ^
bootstrap/kubeadm/api/v1beta1/kubeadm_types.go:697:2: commentstart: field Secret is missing godoc comment (kal)
        Secret string `json:"-"`
        ^

sivchari · 2025-02-19T07:47:10Z

Hi @tsuzu
Thank you for trying to run KAL in advance.
This is part of #11834 . It seems to be good to start enabling rules other than commentstart.

Hi @JoelSpeed
@tsuzu has already worked to resolve false positive about omitted field on JoelSpeed/kal#39
So I'd work on other two issues. Can I take on it ?

Thanks.

JoelSpeed · 2025-02-19T11:15:57Z

Omitted fields should be ignored (Ignore if json tag is -

Agreed, these can be ignored

Non-API resources (can be skipped with path)

What makes these non-api? This goes back to our project to clean up the API packages to just contain APIs, but yes, if these are not API sourced, then we should skip them.

Perhaps these could be updated anyway, seems there's a small number?

I have a plan to implement "pathing" within KAL, so that we can find a list of all paths under which a field/type is used. If there are no paths to the field/type, then skipping linting the type is probably appropriate, that's on the backburner for now though.

Inline non-API resources

What is the difference between these and the above types? These are types we construct to pass to kubeadm?

So I'd work on other two issues. Can I take on it ?

I assume that's fine, but lets make sure we understand what these types are first and whether @tsuzu wants to resolve them

sivchari · 2025-02-19T11:31:52Z

I assume that's fine, but lets make sure we understand what these types are first and whether @tsuzu wants to resolve them

Agreed, thanks.

sbueringer · 2025-02-19T16:48:09Z

@tsuzu

Let's fix the findings in exp/runtime/hooks/api/v1alpha1/ we basically already treat the types there like API types even though they are not technically part of a Kubernetes API type (they are part of our Runtime Extension API though)
Let's also fix cmd/clusterctl/api/v1alpha3/metadata_type.go:32, not worth the exception

Inline non-API resources

@JoelSpeed If I read this correctly it's about these types:

cluster-api/bootstrap/kubeadm/api/v1beta1/kubeadm_types.go

Lines 289 to 295 in be7e672

    
           Name                  string            `json:"name,omitempty"` 
        
           CRISocket             string            `json:"criSocket,omitempty"` 
        
           Taints                []corev1.Taint    `json:"taints"` 
        
           KubeletExtraArgs      map[string]string `json:"kubeletExtraArgs,omitempty"` 
        
           IgnorePreflightErrors []string          `json:"ignorePreflightErrors,omitempty"` 
        
           ImagePullPolicy       string            `json:"imagePullPolicy,omitempty"` 
        
           ImagePullSerial       *bool             `json:"imagePullSerial,omitempty"`

Doesn't seem to make sense to add comments to anonymous structs that we use within a MarshalJSON func :)

JoelSpeed · 2025-02-19T17:28:40Z

Doesn't seem to make sense to add comments to anonymous structs that we use within a MarshalJSON func :)

Ahh missed those, yeah that's probably something we can fix on the KAL side, i wouldn't expect it to be picking up anonymous functions in structs (though the custom marshalling there is also pretty wild/out there for an API package 👀 Would love to understand why that was written)

sbueringer · 2025-02-19T17:34:53Z

That was written because of limitations of the json package. Please see:

cluster-api/bootstrap/kubeadm/api/v1beta1/kubeadm_types.go

Lines 276 to 284 in be7e672

    
           // MarshalJSON marshals NodeRegistrationOptions in a way that an empty slice in Taints is preserved. 
        
           // Taints are then rendered as: 
        
           // * nil => omitted from the marshalled JSON 
        
           // * [] => rendered as empty array (`[]`) 
        
           // * [regular-array] => rendered as usual 
        
           // We have to do this as the regular Golang JSON marshalling would just omit 
        
           // the empty slice (xref: https://github.com/golang/go/issues/22480). 
        
           // Note: We can't re-use the original struct as that would lead to an infinite recursion. 
        
           // Note: The structs in this func have to be kept in sync with the NodeRegistrationOptions struct.

JoelSpeed · 2025-02-19T17:54:48Z

Why do we need the rendered taints: []?

sbueringer · 2025-02-19T17:56:41Z

Why do we need the rendered taints: []?

Because it is the only way to tell kubeadm to not set any taints

xref: https://github.com/kubernetes/kubernetes/blob/7edc7fbd47ed591d0c4e763a25138248140c4dcc/cmd/kubeadm/app/apis/kubeadm/v1beta4/types.go#L253-L256

tsuzu · 2025-02-20T14:49:17Z

Let's fix the findings in exp/runtime/hooks/api/v1alpha1/ we basically already treat the types there like API types even though they are not technically part of a Kubernetes API type (they are part of our Runtime Extension API though)
Let's also fix cmd/clusterctl/api/v1alpha3/metadata_type.go:32, not worth the exception

I got it! I'll add comments for them.

Update: Added

Doesn't seem to make sense to add comments to anonymous structs that we use within a MarshalJSON func :)

Thank you for the explanation. That's I wanted to say.
https://github.com/tsuzu/cluster-api/blob/bae1ffc085bde95aea356bdcc850ecbc7a50b495/hack/tools/staticchecker/internal/analyzer/godocprefix.go#L44
We can check whether ast.GenDecl is a parent of ast.Struct. Then, I'll also prepare a PR for this.

chrischdi · 2025-02-20T16:50:38Z

api/v1beta1/cluster_types.go

@@ -1047,7 +1047,9 @@ type Cluster struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`

-	Spec   ClusterSpec   `json:"spec,omitempty"`
+	// spec is the desired state of Cluster


Suggested change

// spec is the desired state of Cluster

// spec is the desired state of Cluster.

Shouldn't these always end in a dot?

Thanks for the review! Fixed with at 1281b4b

chrischdi · 2025-02-20T16:50:44Z

api/v1beta1/cluster_types.go

@@ -1180,7 +1182,8 @@ func (f ClusterIPFamily) String() string {
 type ClusterList struct {
 	metav1.TypeMeta `json:",inline"`
 	metav1.ListMeta `json:"metadata,omitempty"`
-	Items           []Cluster `json:"items"`
+	// items is the list of Cluster


Suggested change

// items is the list of Cluster

// items is the list of Clusters.

I think it should always be plural for the items case. (also the dot?!)

sivchari

It seems the comment that other Spec has also starts with status.

sivchari · 2025-02-21T01:10:28Z

api/v1beta1/cluster_types.go

@@ -1047,7 +1047,9 @@ type Cluster struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`

-	Spec   ClusterSpec   `json:"spec,omitempty"`
+	// status is the desired state of Cluster.


Suggested change

// status is the desired state of Cluster.

// spec is the desired state of Cluster.

Ah, sorry. Fixed.
Thank you!

sbueringer · 2025-02-21T07:36:33Z

/ok-to-test

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/documentation Issues or PRs related to documentation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 18, 2025

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 18, 2025

tsuzu mentioned this pull request Feb 18, 2025

Refactor godocs for API fields to start with the serialised versions #11238

Open

tsuzu marked this pull request as ready for review February 18, 2025 21:43

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 18, 2025

chrischdi reviewed Feb 20, 2025

View reviewed changes

tsuzu force-pushed the fix-spec-status-items-comments branch from 1f4d683 to 1281b4b Compare February 20, 2025 18:03

tsuzu mentioned this pull request Feb 20, 2025

fix: anonymous/inaccessible structs for commentstart JoelSpeed/kal#40

Merged

sivchari reviewed Feb 21, 2025

View reviewed changes

Add comments for spec/status/items

a0107cf

tsuzu force-pushed the fix-spec-status-items-comments branch from 1281b4b to a0107cf Compare February 21, 2025 06:04

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 21, 2025

JoelSpeed mentioned this pull request Feb 27, 2025

API Linting Tracking Issue #11834

Open

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Add comments for spec/status/items #11870

🌱 Add comments for spec/status/items #11870

tsuzu commented Feb 18, 2025 •

edited

Loading

k8s-ci-robot commented Feb 18, 2025

k8s-ci-robot commented Feb 18, 2025

tsuzu commented Feb 18, 2025 •

edited

Loading

sivchari commented Feb 19, 2025

JoelSpeed commented Feb 19, 2025

sivchari commented Feb 19, 2025

sbueringer commented Feb 19, 2025 •

edited

Loading

JoelSpeed commented Feb 19, 2025

sbueringer commented Feb 19, 2025

JoelSpeed commented Feb 19, 2025

sbueringer commented Feb 19, 2025 •

edited

Loading

tsuzu commented Feb 20, 2025 •

edited

Loading

chrischdi Feb 20, 2025 •

edited

Loading

tsuzu Feb 20, 2025

chrischdi Feb 20, 2025 •

edited

Loading

sivchari left a comment

sivchari Feb 21, 2025

tsuzu Feb 21, 2025

sbueringer commented Feb 21, 2025

	// spec is the desired state of Cluster
	// spec is the desired state of Cluster.

	// items is the list of Cluster
	// items is the list of Clusters.

	// status is the desired state of Cluster.
	// spec is the desired state of Cluster.

🌱 Add comments for spec/status/items #11870

Are you sure you want to change the base?

🌱 Add comments for spec/status/items #11870

Conversation

tsuzu commented Feb 18, 2025 • edited Loading

k8s-ci-robot commented Feb 18, 2025

k8s-ci-robot commented Feb 18, 2025

tsuzu commented Feb 18, 2025 • edited Loading

sivchari commented Feb 19, 2025

JoelSpeed commented Feb 19, 2025

sivchari commented Feb 19, 2025

sbueringer commented Feb 19, 2025 • edited Loading

JoelSpeed commented Feb 19, 2025

sbueringer commented Feb 19, 2025

JoelSpeed commented Feb 19, 2025

sbueringer commented Feb 19, 2025 • edited Loading

tsuzu commented Feb 20, 2025 • edited Loading

chrischdi Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

tsuzu Feb 20, 2025

Choose a reason for hiding this comment

chrischdi Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

sivchari left a comment

Choose a reason for hiding this comment

sivchari Feb 21, 2025

Choose a reason for hiding this comment

tsuzu Feb 21, 2025

Choose a reason for hiding this comment

sbueringer commented Feb 21, 2025

tsuzu commented Feb 18, 2025 •

edited

Loading

tsuzu commented Feb 18, 2025 •

edited

Loading

sbueringer commented Feb 19, 2025 •

edited

Loading

sbueringer commented Feb 19, 2025 •

edited

Loading

tsuzu commented Feb 20, 2025 •

edited

Loading

chrischdi Feb 20, 2025 •

edited

Loading

chrischdi Feb 20, 2025 •

edited

Loading