Automate linter registration to prevent unknown linter errors #205
Conversation
Add a script to automatically generate pkg/registration/doc.go from linter packages under pkg/analysis/. This prevents the common issue where adding a new linter but forgetting to register it causes "unknown linter" errors. Changes: - Add hack/generate-registration.sh to auto-detect and register linters - Add make generate-registration target - Add verify step to CI workflow - Register previously missing minlength and namingconventions linters
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sivchari The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
approved
| # Find all linter packages (directories with initializer.go, excluding the shared initializer package) | ||
| linters=() | ||
| while IFS= read -r -d '' file; do | ||
| dir=$(dirname "$file") | ||
| linter=$(basename "$dir") | ||
| # Skip the shared initializer package and registry | ||
| if [[ "$linter" != "initializer" && "$linter" != "registry" && "$linter" != "helpers" && "$linter" != "utils" ]]; then | ||
| linters+=("$linter") | ||
| fi | ||
| done < <(find "${ANALYSIS_DIR}" -name "initializer.go" -print0 | sort -z) | ||
|
|
||
| # Sort linters alphabetically | ||
| IFS=$'\n' sorted_linters=($(sort <<<"${linters[*]}")); unset IFS |
There was a problem hiding this comment.
It seems like this could probably be replaced with something like:
go list ./pkg/analysis/... | grep -v "utils" | grep -v "initializer" | grep -v "helpers" | grep -v "registry" | grep -v "analysis$" | grep -v "errors"?
There was a problem hiding this comment.
Is there a way to check inside each package to look for an expected initializer? Since that's what we need here? Sorting as an exclusion list may not age well
There was a problem hiding this comment.
Something like:
go list -json ./pkg/analysis/... | jq -rs '.[] | select(.GoFiles | index("initializer.go")) | .ImportPath'should work
There was a problem hiding this comment.
Although we probably actually want:
go list -json ./pkg/analysis/... | jq -rs '.[] | select(.GoFiles | index("analyzer.go")) | .ImportPath'to only pick up ones with an analyzer.go file
There was a problem hiding this comment.
Better yet!
go list -json ./pkg/analysis/... | jq -rs '.[] | select((.GoFiles | index("initializer.go")) and (.GoFiles | index("analyzer.go"))) | .ImportPath'pick up both analyzer.go and initializer.go 😅
| # Sort linters alphabetically | ||
| IFS=$'\n' sorted_linters=($(sort <<<"${linters[*]}")); unset IFS | ||
|
|
||
| # Generate the doc.go file |
There was a problem hiding this comment.
Do we actually need to do a full generation here?
Instead, could we compare the output of something like:
go list -json | jq -r '.Imports | .[]'for verification?
There was a problem hiding this comment.
Turning this into a test vs a generator makes sense to me
There was a problem hiding this comment.
Clarifying: I recognize that this PR is meant for generation - I'm back and forth on whether or not we should do automatic generation at the moment or just verification, hence this comment.
We've had quite a bit of changes take place to the utilities and structure of the linters package over time as we add new linters - some of which may or may not make sense to include for registration.
I'd rather not have automatic generation of something that shouldn't be there slip through but maybe the impact of that would be pretty minimal.
My thinking with this being simple verification logic is that it becomes an explicit decision to include/disclude something from the registration as opposed to a blind make generate-registration.
There was a problem hiding this comment.
I whipped up a simple script that just does verification like:
#! /bin/bash
linters=$(go list -json ./pkg/analysis/... | jq -rs '.[] | select((.GoFiles | index("initializer.go")) and (.GoFiles | index("analyzer.go"))) | .ImportPath')
registered=$(go list -json ./pkg/registration | jq -r '.Imports | .[]')
if [[ "${linters[*]}" != "${registered[*]}" ]]; then
echo "Not all linters are registered, is this expected?"
echo "Calculated diff (+++ is available linters, --- is registered linters):"
echo "----------------"
diff -Naup <(echo "${linters}") <(echo "${registered}")
fiOutput on main looks like:
hack/check-registration.sh
Not all linters are registered, is this expected?
Calculated diff (+++ is available linters, --- is registered linters):
----------------
--- /dev/fd/63 2025-12-18 09:16:25
+++ /dev/fd/62 2025-12-18 09:16:25
@@ -9,8 +9,6 @@ sigs.k8s.io/kube-api-linter/pkg/analysis/maxlength
sigs.k8s.io/kube-api-linter/pkg/analysis/integers
sigs.k8s.io/kube-api-linter/pkg/analysis/jsontags
sigs.k8s.io/kube-api-linter/pkg/analysis/maxlength
-sigs.k8s.io/kube-api-linter/pkg/analysis/minlength
-sigs.k8s.io/kube-api-linter/pkg/analysis/namingconventions
sigs.k8s.io/kube-api-linter/pkg/analysis/nobools
sigs.k8s.io/kube-api-linter/pkg/analysis/nodurations
sigs.k8s.io/kube-api-linter/pkg/analysis/nofloats
make: *** [check-registration] Error 1
|
/approve cancel to prevent accidental merge We probably want to switch to a test like #205 (comment) |
4 participants
Add a script to automatically generate pkg/registration/doc.go from linter packages under pkg/analysis/. This prevents the common issue where adding a new linter but forgetting to register it causes "unknown linter" errors.
I noticed that
namingconventionlinter was also missed.Close #203