Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 DNM: Add KAL linter for linting API conventions #11733

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
+81 −1
Draft

🌱 DNM: Add KAL linter for linting API conventions #11733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d22ea34
Add custom KAL golangci-lint in-place of golangci-lint
JoelSpeed Jan 22, 2025
000d87f
Move KAL to separate golangci-lint run
JoelSpeed Jan 22, 2025
1dce912
Add KAL to lint workflow
JoelSpeed Jan 22, 2025
ab11d8e
DNM: Lint v1beta1 folder
JoelSpeed Jan 22, 2025
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
2 changes: 2 additions & 0 deletions .github/workflows/pr-golangci-lint.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,5 @@ jobs:
version: v1.63.4
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
args: --out-format=colored-line-number
working-directory: ${{matrix.working-directory}}
- name: Lint API
run: GOLANGCI_LINT_EXTRA_ARGS=--out-format=colored-line-number make lint-api
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory we could add --new-from-rev=main to allow this to lint only changes since the main branch, which would allow us to start enforcing the linting rules for new changes to older APIs as well 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me

55 changes: 55 additions & 0 deletions .golangci-kal.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
run:
timeout: 10m
go: "1.23"
allow-parallel-runners: true

linters:
disable-all: true
enable:
- kal # linter for Kube API conventions

linters-settings:
custom:
kal:
type: "module"
description: KAL is the Kube-API-Linter and lints Kube like APIs based on API conventions and best practices.
settings:
linters:
enable:
- "conditions" # Ensure conditions have the correct json tags and markers.
Copy link
Member

Choose a reason for hiding this comment

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

+1

Seems like the linter also enforces:

// +patchStrategy=merge
// +patchMergeKey=type

Does this make sense for CRDs?

Obviously we can't fix this finding for the old condition fields that use clusterv1.Conditions

- "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.
- "maxlength" # Ensure all strings and arrays have maximum lengths/maximum items.
Copy link
Member

Choose a reason for hiding this comment

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

+1 I would be even okay with picking some sane max values even for existing fields

- "nobools" # Bools do not evolve over time, should use enums instead.
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about this one.

Mostly because I'm not sure if every bool can be intuitively expressed as an enum instead. I think we have a lot of fields that worked quite well as a bool and we have no need/plans to evolve them

Additionally, general edge case, in CABPK / KCP we ~ inherit a significant part of our API from kubeadm. I wouldn't like to diverge from these upstream APIs to avoid this finding.

- "nophase" # Phase fields are discouraged by the Kube API conventions, use conditions instead.
Copy link
Member

Choose a reason for hiding this comment

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

This one will be controversial. We initially wanted to drop phase, but then later realized that the phases are actually useful to users because they quickly give users an idea about the phase a Cluster/Machine is in without having to parse through a number of conditions

- "optionalorrequired" # Every field should be marked as `+optional` or `+required`.
- "requiredfields" # Required fields should not be pointers, and should not have `omitempty`.
- "statussubresource" # All root objects that have a `status` field should have a status subresource.
disable:
- "*" # We will manually enable new linters after understanding the impact. Disable all by default.
lintersConfig:
conditions:
isFirstField: Warn # Require conditions to be the first field in the status struct.
Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me, just curious. Why?

useProtobuf: Ignore # We don't use protobuf, so missing protobuf tags are ignored.
# 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`.
Copy link
Member

Choose a reason for hiding this comment

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

Definitely in favor of using required / optional over the kubebuilder markers

# 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.

issues:
exclude-files:
- "zz_generated.*\\.go$"
- "vendored_openapi\\.go$"
# We don't want to invest time to fix new linter findings in old API types.
- "internal/apis/.*"
max-same-issues: 0
max-issues-per-linter: 0
exclude-rules:
# KAL should only run on API folders.
- path-except: "api/v1beta1/*"
Copy link
Member

Choose a reason for hiding this comment

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

We unfortunately have some more API folders (just a comment for before we merge)

linters:
- kal
1 change: 1 addition & 0 deletions .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,7 @@ linters-settings:
- name: constant-logical-expr
goconst:
ignore-tests: true

issues:
exclude-files:
- "zz_generated.*\\.go$"
Expand Down
18 changes: 17 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,10 @@ GOLANGCI_LINT_VER := $(shell cat .github/workflows/pr-golangci-lint.yaml | grep
GOLANGCI_LINT := $(abspath $(TOOLS_BIN_DIR)/$(GOLANGCI_LINT_BIN)-$(GOLANGCI_LINT_VER))
GOLANGCI_LINT_PKG := github.com/golangci/golangci-lint/cmd/golangci-lint

GOLANGCI_LINT_KAL_BIN := golangci-lint-kal
GOLANGCI_LINT_KAL_VER := $(shell cat ./hack/tools/.custom-gcl.yaml | grep name: | sed 's/name: golangci-lint-kal-//')
GOLANGCI_LINT_KAL := $(abspath $(TOOLS_BIN_DIR)/$(GOLANGCI_LINT_KAL_BIN)-$(GOLANGCI_LINT_KAL_VER))

GOVULNCHECK_BIN := govulncheck
GOVULNCHECK_VER := v1.1.4
GOVULNCHECK := $(abspath $(TOOLS_BIN_DIR)/$(GOVULNCHECK_BIN)-$(GOVULNCHECK_VER))
Expand Down Expand Up @@ -660,11 +664,12 @@ generate-test-infra-prowjobs: $(PROWJOB_GEN) ## Generates the prowjob configurat
##@ lint and verify:

.PHONY: lint
lint: $(GOLANGCI_LINT) ## Lint the codebase
lint: $(GOLANGCI_LINT) $(GOLANGCI_LINT_KAL) ## Lint the codebase
$(GOLANGCI_LINT) run -v $(GOLANGCI_LINT_EXTRA_ARGS)
cd $(TEST_DIR); $(GOLANGCI_LINT) run --path-prefix $(TEST_DIR) --config $(ROOT_DIR)/.golangci.yml -v $(GOLANGCI_LINT_EXTRA_ARGS)
cd $(TOOLS_DIR); $(GOLANGCI_LINT) run --path-prefix $(TOOLS_DIR) --config $(ROOT_DIR)/.golangci.yml -v $(GOLANGCI_LINT_EXTRA_ARGS)
./scripts/lint-dockerfiles.sh $(HADOLINT_VER) $(HADOLINT_FAILURE_THRESHOLD)
$(GOLANGCI_LINT_KAL) run -v --config $(ROOT_DIR)/.golangci-kal.yml $(GOLANGCI_LINT_EXTRA_ARGS)

.PHONY: lint-dockerfiles
lint-dockerfiles:
Expand All @@ -674,6 +679,14 @@ lint-dockerfiles:
lint-fix: $(GOLANGCI_LINT) ## Lint the codebase and run auto-fixers if supported by the linter
GOLANGCI_LINT_EXTRA_ARGS=--fix $(MAKE) lint

.PHONY: lint-api
lint-api: $(GOLANGCI_LINT_KAL)
$(GOLANGCI_LINT_KAL) run -v --config $(ROOT_DIR)/.golangci-kal.yml $(GOLANGCI_LINT_EXTRA_ARGS)

.PHONY: lint-api-fix
lint-api-fix: $(GOLANGCI_LINT_KAL)
GOLANGCI_LINT_EXTRA_ARGS=--fix $(MAKE) lint-api

.PHONY: tiltfile-fix
tiltfile-fix: ## Format the Tiltfile
TRACE=$(TRACE) ./hack/verify-starlark.sh fix
Expand Down Expand Up @@ -1501,6 +1514,9 @@ $(GINKGO): # Build ginkgo from tools folder.
$(GOLANGCI_LINT): # Build golangci-lint from tools folder.
GOBIN=$(TOOLS_BIN_DIR) $(GO_INSTALL) $(GOLANGCI_LINT_PKG) $(GOLANGCI_LINT_BIN) $(GOLANGCI_LINT_VER)

$(GOLANGCI_LINT_KAL): $(GOLANGCI_LINT) # Build golangci-lint-kal from custom configuration.
cd $(TOOLS_DIR); $(GOLANGCI_LINT) custom

$(GOVULNCHECK): # Build govulncheck.
GOBIN=$(TOOLS_BIN_DIR) $(GO_INSTALL) $(GOVULNCHECK_PKG) $(GOVULNCHECK_BIN) $(GOVULNCHECK_VER)

Expand Down
6 changes: 6 additions & 0 deletions hack/tools/.custom-gcl.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
version: v1.63.4
name: golangci-lint-kal-v1.63.4
destination: ./bin
plugins:
- module: 'github.com/JoelSpeed/kal'
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm understanding correctly, if/when the project's accepted in kubernetes-sigs, this file is the only thing that needs to change, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, we would need to update this to point to the new source. There may of course be other changes over time since this project is early in development, but, I would hope that we can keep this fairly compatible with what we are doing here and now

My aim at least is that it stays as a golangci-lint plugin until and unless it replaces the flexibility of the configuration already implemented by golangci-lint. But there's an awful lot of functionality there and I don't see much value in re-implementing that.

version: v0.0.0-20250121143106-304c215e474e
Loading