🌱 Enable jsontags lint of KAL #11890

sivchari · 2025-02-22T09:15:04Z

What this PR does / why we need it:

This is part of #11834
Enabled jsontags linter

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

sbueringer · 2025-02-24T17:51:37Z

.golangci-kal.yml

@@ -26,7 +26,7 @@ linters-settings:
          # Linters below this line are disabled, pending conversation on how and when to enable them.
          # - "commentstart" # Ensure comments start with the serialized version of the field name.
          # - "integers" # Ensure only int32 and int64 are used for integers.
-          # - "jsontags" # Ensure every field has a json tag.
+          - "jsontags" # Ensure every field has a json tag.


Please move this up below conditions. This is the section for the disabled linters

sbueringer · 2025-02-24T17:53:17Z

.golangci-kal.yml

@@ -61,7 +61,36 @@ issues:
  - path-except: "api/*"
    linters:
      - kal
+  # apiwarnings contains a `api` characters in the path, but it's not an API folder.
+  - path: "util/apiwarnings/*"


I don't follow why apiwarnings/ is apparently matching api/*

@JoelSpeed Am I missing something? :)

Hmm, it doesn't appear to match with the path exception so I'm not sure why this package would be picked up, the pattern is api/*

cluster-api/.golangci-kal.yml

Line 61 in 15ce36f

- path-except: "api/*"

Definitely shows these findings if I drop this exclude

util/apiwarnings/handler.go:33:2: jsontags: field Logger is missing json tag (kal) Logger logr.Logger ^ util/apiwarnings/handler.go:37:2: jsontags: field Expressions is missing json tag (kal) Expressions []regexp.Regexp ^

Works fine with this

# KAL should only run on API folders. - path-except: "api//*" linters: - kal

Not sure if I want to know why, @sivchari can you drop this exclude and adjust the path-except above please? Thx! :)

We should probably also change all the other / into // in other path values

Not sure if I want to know why

How interesting 🤔

Very unintuitive

Thx!
How confusing :(

sbueringer · 2025-02-24T17:55:26Z

.golangci-kal.yml

+  - path: "api/v1beta1/*"
+    text: "field XPreserveUnknownFields json tag does not match pattern"
+    linters:
+      - kal
+  - path: "api/v1beta1/*"
+    text: "field XValidations json tag does not match pattern"
+    linters:
+      - kal
+  - path: "api/v1beta1/*"
+    text: "field XMetadata json tag does not match pattern"
+    linters:
+      - kal
+  - path: "api/v1beta1/*"
+    text: "field XIntOrString json tag does not match pattern"
+    linters:
+      - kal


Let's merge them all into one regex match
Let's also ignore them in general and not only for v1beta1
Let's add a comment that we intentionally align these fields to the corresponding fields in CustomResourceDefinitions

+1 this makes sense to me

.golangci-kal.yml

JoelSpeed · 2025-02-26T10:34:59Z

.golangci-kal.yml

    linters:
      - kal
-  - path: "api/v1beta1/*|api/v1alpha1/*"
+  - path: "api//*"


This one still wants to be v1beta1 so that when we introduce new v1beta2 APIs this is enforced

JoelSpeed · 2025-02-26T10:35:46Z

.golangci-kal.yml

-      - kal
-  - path: "api/v1beta1/*"
-    text: "field XIntOrString json tag does not match pattern"
+    source: "X.*"


I think this is too broad, can we be specific about the field names we are excepting here please

Exactly. fixed.

JoelSpeed · 2025-02-26T10:36:38Z

.golangci-kal.yml

    linters:
      - kal
+  # These reporting are false positives.
+  # When the following issue is fixed, remove the exclusion.
+  # https://github.com/JoelSpeed/kal/issues/43


I don't actually understand what that issue is? The PR merged to fix this no?

Could it be that we just didn't bump KAL since the fix was merged?

I use the 1ad4d048de47e0fa31c48e8f5d2ebe9eded0f4e4 in e48fa3c.
I expect this rule is removed since the extractjsontags analyzer is fixed, but this report are still be existing.
Am I missing something ?

Ok I see, we fixed the comments linter, but not the json tags linter itself, will need a small fix there too

I open the pr (this is wip, yet) to fix jsontags linter.

JoelSpeed · 2025-02-26T11:15:23Z

.golangci-kal.yml

+          jsonTags:
+            jsonTagRegex: "^[a-z][a-z0-9]*(?:[A-Z][a-z0-9]*)*$" # The default regex is appropriate for our use case.


Do we want to uncomment this, or shall we just leave it commented so that if the default configuration changes over time we continue to use the default?

The CI will be failed if KAL changes default configuration. Then we'll end up adding the this configuration to manage the compability.
So I think it's better to keep this configuration.

CI will only fail if we bump KAL on the PR where we bump KAL

@JoelSpeed How do we keep the documented default values here up-to-date if they change in KAL?

(In general I'm in favor of using the default value here from KAL)

@JoelSpeed How do we keep the documented default values here up-to-date if they change in KAL?

Open to suggestions, I don't know at the moment.

I'm expecting in the not too distant future to move the project to kube-sigs. Once it's moved, and we've had agreement from sig-apimachinery that we are happy with the structure and defaults etc, that we probably will start treating the project like a v1 and therefore breaking changes won't be possible.

So I guess, as we update kal, we add the new sections in to show what the new config might be, but I don't have a great/automatic suggestion at the moment

All good. We can just sync the defaults once we reach "v1"

Agreed. thx!

JoelSpeed · 2025-02-27T10:12:16Z

.golangci-kal.yml

@@ -39,13 +39,11 @@ linters-settings:
            isFirstField: Warn # Require conditions to be the first field in the status struct.
            usePatchStrategy: Forbid # Conditions should not use the patch strategy on CRDs.
            useProtobuf: Forbid # We don't use protobuf, so protobuf tags are not required.
-        # jsonTags:


I think we should leave this section commented out so that we can easily see what configuration is available to us, rather than removing the comments. It may help us in the future when trying to work out why something isn't behaving in the way we might expect

JoelSpeed · 2025-02-27T11:38:17Z

.golangci-kal.yml

+          # jsontags:
+          #    jsonTagRegex: "^[a-z][a-z0-9]*(?:[A-Z][a-z0-9]*)*$" # The default regex is appropriate for our use case.
+          # optionalOrRequired:
+          #   preferredOptionalMarker: optional | kubebuilder:validation:Optional # The preferred optional marker to use, fixes will suggest to use this marker. Defaults to `optional`.
+          #   preferredRequiredMarker: required | kubebuilder:validation:Required # The preferred required marker to use, fixes will suggest to use this marker. Defaults to `required`.
+          # requiredFields:
+          #   pointerPolicy: Warn | SuggestFix # Defaults to `SuggestFix`. We want our required fields to not be pointers.


This section has all moved, can we unmove it so that it's at the original indentation level prior to this PR?

Sorry, fixed.

JoelSpeed · 2025-02-27T13:11:50Z

Could we squash some of the fixup style commits please so that we have a set of logical, atomic commits to merge?

JoelSpeed · 2025-03-03T10:07:43Z

You might have better luck with the rebases if we were to order the list of enabled linters alphabetically again 🤔

sbueringer · 2025-03-03T11:44:30Z

@JoelSpeed seems like KAL is also looking at interfaces

JoelSpeed · 2025-03-03T11:46:16Z

@sbueringer Yes, it appears there was a regression (see JoelSpeed/kal#48) and then I'm fixing that in JoelSpeed/kal#49

Lets hold this one until 49 is merged please

JoelSpeed · 2025-03-04T14:39:10Z

The KAL fix is merged, so if we can get this updated with the latest KAL, I think we can move forward to get this merged

Signed-off-by: sivchari <[email protected]>

JoelSpeed · 2025-03-04T15:28:19Z

/lgtm

Thanks @sivchari

k8s-ci-robot · 2025-03-04T15:28:25Z

LGTM label has been added.

Git tree hash: 0ada23263c0b3c55a95a0d82bdb24e0f345436dc

sbueringer · 2025-03-04T15:31:59Z

Thx!

/lgtm
/approve

k8s-ci-robot · 2025-03-04T15:32:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sbueringer]

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 added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 22, 2025

sbueringer reviewed Feb 24, 2025

View reviewed changes

sbueringer added the area/misc Issues or PRs not related to any other area label Feb 24, 2025

k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Feb 24, 2025

JoelSpeed reviewed Feb 26, 2025

View reviewed changes

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 26, 2025

sivchari mentioned this pull request Feb 26, 2025

jsontags: Ignore fields of json:"-" JoelSpeed/kal#43

Closed

JoelSpeed reviewed Feb 26, 2025

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 26, 2025

sivchari force-pushed the enable-jsontags branch from e48fa3c to 4ebf4c1 Compare February 27, 2025 05:06

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 27, 2025

JoelSpeed mentioned this pull request Feb 27, 2025

API Linting Tracking Issue #11834

Open

16 tasks

JoelSpeed reviewed Feb 27, 2025

View reviewed changes

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 27, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 27, 2025

sivchari force-pushed the enable-jsontags branch from 6cf1f25 to 165ed56 Compare March 3, 2025 09:51

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2025

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 3, 2025

sivchari force-pushed the enable-jsontags branch from 165ed56 to d80cc3d Compare March 3, 2025 10:25

sivchari force-pushed the enable-jsontags branch from d80cc3d to aa61dbd Compare March 4, 2025 15:06

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2025

enable jsontags rule

06e86de

Signed-off-by: sivchari <[email protected]>

sivchari force-pushed the enable-jsontags branch from aa61dbd to 06e86de Compare March 4, 2025 15:06

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 4, 2025

k8s-ci-robot assigned JoelSpeed Mar 4, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2025

k8s-ci-robot assigned sbueringer Mar 4, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 4, 2025

k8s-ci-robot merged commit dc17a9b into kubernetes-sigs:main Mar 4, 2025
19 checks passed

k8s-ci-robot added this to the v1.10 milestone Mar 4, 2025

		jsonTags:
		jsonTagRegex: "^[a-z][a-z0-9](?:[A-Z][a-z0-9])*$" # The default regex is appropriate for our use case.

🌱 Enable jsontags lint of KAL #11890

🌱 Enable jsontags lint of KAL #11890

Conversation

sivchari commented Feb 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sivchari Feb 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed commented Feb 27, 2025

JoelSpeed commented Mar 3, 2025

sbueringer commented Mar 3, 2025

JoelSpeed commented Mar 3, 2025

JoelSpeed commented Mar 4, 2025

JoelSpeed commented Mar 4, 2025

k8s-ci-robot commented Mar 4, 2025

sbueringer commented Mar 4, 2025

k8s-ci-robot commented Mar 4, 2025

sivchari Feb 26, 2025 •

edited

Loading