Skip to content

Add makefile entries for api-lint #1384

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

Merged
merged 1 commit into from
Sep 16, 2025
Merged

Add makefile entries for api-lint #1384

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 2 additions & 6 deletions .github/workflows/kal.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,5 @@ jobs:
persist-credentials: false
- name: Set up Go
uses: actions/setup-go@d35c59abb061a4a6fb18e82ac0862c26744d6ab5 # tag=v5.5.0
- name: Install Golang CI Lint
run: go install github.com/golangci/golangci-lint/v2/cmd/[email protected]
- name: Build KAL
run: golangci-lint custom
- name: run api linter
run: ./bin/golangci-kube-api-linter run -c ./.golangci-kal.yml ./...
- name: Run API Linter
Copy link
Contributor

Choose a reason for hiding this comment

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

make verify runs as a pre submit check on every PR. now that api-lint was added to the makefile, do you think this workflow is needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to keep it on Github action as:

  • It is faster than prow job (we can remove from make verify later or make prow use a different target, as make verify was actually my attempt to add it to the developer workflow as well
  • The github action is able to annotate the PR directly telling where are the files (as per the screenshot above)
    wdyt? I think for now it doesn't hurt to have both prow job and the github action running, and we do some followup clean up on the prow job

Copy link
Contributor

Choose a reason for hiding this comment

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

personally I like to have it in makefile without github workflow which allow users to run it locally in their envs with no surprises later in ci. as you probably noticed we have no github workflows, everything runs through the make verify which makes it repeatable in local envs.

I think it’s cleaner, but not feeling strongly about it.

Copy link
Member

Choose a reason for hiding this comment

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

I think the benefits of using GitHub actions here (remarkably fast/lightweight presubmits, clearer feedback directly on the PR) has outweighed any potential disadvantages in our experience in Gateway API. I agree that we should also aim to have a make task that can verify everything locally as well, but would hate to require the structure of our presubmits to directly match that as it could become quite slow over time. It can be much more efficient to split out parts of the verification into separate presubmits.

Copy link
Contributor

@nirrozenbaum nirrozenbaum Aug 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@robscott you touched two different points:

  1. running different parts of verification in separate presubmit checks for parallelism. that could be done using make commands and just adding make api-lint as another pre-submit check that runs in parallel to make verify. this is easy to do.
  2. running this specific check in github action, which is faster than make command.

let's assume we go with parallelism, so it's either we run another make command in parallel or using github action.
now the question is whether the fact that the action is faster is worth having it (github actions cannot run in local envs, so checks may drift as we make progress).
since we anyway have to wait for the longest pre-submit check which is the tests themselves, when looking at the tradeoff I prefer having an identical setup or at least a way to run it identically locally (e.g., make verify api-lint) rather than having a super fast action, but the other pre-submit checks take long.

but again, not feeling very strongly about it. if you feel strongly about having a github action, let's go with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

As a suggestion, I can remove the api-lint from verify target, and make the github action call make api-lint directly.

My addition of api-lint to verify was mostly as @danehans raised correctly that as a developer, sometimes I just do a make verify on the repo and expect that all of the validations run.

We can probably add to some doc that the api-lint is a different target, or eventually create a new target that should be called by prow, and will contain all verify targets but api-lint.

So a developer doing make verify will get the full package, the CI doing make ci-verify will do all but api-lint, and api-lint is called by github action.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO api-lint should be called from the verify target to simplify the dev UX.

run: make api-lint
12 changes: 11 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,8 +160,12 @@ lint-fix: golangci-lint ## Run golangci-lint linter and perform fixes
ci-lint: golangci-lint
$(GOLANGCI_LINT) run --timeout 15m0s

.PHONY: api-lint
api-lint: golangci-api-lint
$(GOLANGCI_API_LINT) run -c .golangci-kal.yml --timeout 15m0s ./...

.PHONY: verify
verify: vet fmt-verify generate ci-lint verify-all
verify: vet fmt-verify generate ci-lint api-lint verify-all
git --no-pager diff --exit-code config api client-go

.PHONY: verify-crds
Expand Down Expand Up @@ -365,6 +369,7 @@ KUSTOMIZE ?= $(LOCALBIN)/kustomize
CONTROLLER_GEN ?= $(LOCALBIN)/controller-gen
ENVTEST ?= $(LOCALBIN)/setup-envtest
GOLANGCI_LINT = $(LOCALBIN)/golangci-lint
GOLANGCI_API_LINT = $(LOCALBIN)/golangci-kube-api-linter
HELM = $(PROJECT_DIR)/bin/helm
YQ = $(PROJECT_DIR)/bin/yq
KUBECTL_VALIDATE = $(PROJECT_DIR)/bin/kubectl-validate
Expand Down Expand Up @@ -399,6 +404,11 @@ golangci-lint: $(GOLANGCI_LINT) ## Download golangci-lint locally if necessary.
$(GOLANGCI_LINT): $(LOCALBIN)
$(call go-install-tool,$(GOLANGCI_LINT),github.com/golangci/golangci-lint/v2/cmd/golangci-lint,$(GOLANGCI_LINT_VERSION))

.PHONY: golangci-api-lint
golangci-api-lint: golangci-lint $(GOLANGCI_API_LINT) ## Download golangci-lint locally if necessary before building KAL
$(GOLANGCI_API_LINT):
$(GOLANGCI_LINT) custom

.PHONY: yq
yq: ## Download yq locally if necessary.
GOBIN=$(PROJECT_DIR)/bin GO111MODULE=on go install github.com/mikefarah/yq/[email protected]
Expand Down