-
Notifications
You must be signed in to change notification settings - Fork 147
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.
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rikatz The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold I will submit a broken change just to guarantee KAL is running |
a13a60e
to
384a5f7
Compare
/hold cancel |
Maybe make the CI use this so you know local and CI invocations are in sync? |
hey Ben, the verify-main prow job uses it, but good catch. I will change the github action one to use |
cab983e
to
2ab6082
Compare
2ab6082
to
e0a7f1a
Compare
run: golangci-lint custom | ||
- name: run api linter | ||
run: ./bin/golangci-kube-api-linter run -c ./.golangci-kal.yml ./... | ||
- name: Run API Linter |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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, asmake 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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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 tomake verify
. this is easy to do. - 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.
There was a problem hiding this comment.
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.
This PR adds the makefile entries for Kube api linter, as discussed during WG Inference meeting.
The linter now can be called locally using
make verify
ormake api-lint
.A lint-fix wasn't added, as it is expected that the fixes are discussed and not automatically fixed.
Fixes: #1380