✨ (go/v4): Add new makefile target to check and validate the linter config #4425
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
needs-ok-to-test
|
Hi @mateusoliveira43. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
camilamacedo86
changed the title
| .PHONY: lint-config | ||
| lint-config: golangci-lint ## Verify golangci-lint linter configuration | ||
| $(GOLANGCI_LINT) config verify | ||
|
|
There was a problem hiding this comment.
We change things for end users, so as it is an addition, we should use ✨ since it should be highlighted in the release notes.
| @@ -18,7 +18,7 @@ issues: | |||
|
|
|||
| linters-settings: | |||
| govet: | |||
| enable=fieldalignment: true | |||
There was a problem hiding this comment.
@mateusoliveira43 this change seems unrelated to the scope right?
Seems that if we change it, then we will start to fail where we did not before
Is that falling under the new target to validate the configuration?
Why do we need to do this change?
If we need to make this change, we need to fix the issues as well
There was a problem hiding this comment.
So, looking CI errors, I think the only complain is indeed fieldalignment errors (I do not know yet why lint-sample failed)
should I disable it?
There was a problem hiding this comment.
We should not disabled the lint-simple
The lint sample has the purpose of lint the test samples and ensuring that the code that we generate with the tool passes the checks.
Why we need to change enable=fieldalignment: true can we please revert that it does not seems part of the scope then, if you wish change it we can do in a follow up where the issues are fixed and we explain why one option should be used instead of another
There was a problem hiding this comment.
Moreover, we need to have a new target for the Makefile that we use for Kubebuilder as well.
It can be here, and we do all by once, for our samples and tooling or can be in a follow up PR
There was a problem hiding this comment.
oh no, I mean disable fieldalignment rule
If you check lint job logs, all errors were related to this rule
I made the change because golangci-lint config verify complained about this configuration
but we can change this part of project Makefile in a separate PR
There was a problem hiding this comment.
undid changes to configuration, just added new command to project Makefile, without calling it in CI
There was a problem hiding this comment.
I made the change because golangci-lint config verify complained about this configuration
Should it not fail in the CI if the lint raises errors and we revert the change?
| .PHONY: lint-config | ||
| lint-config: golangci-lint ## Verify golangci-lint linter configuration | ||
| $(GOLANGCI_LINT) config verify | ||
|
|
||
| .PHONY: lint | ||
| lint: golangci-lint ## Run golangci-lint linter | ||
| $(GOLANGCI_LINT) run |
There was a problem hiding this comment.
We have an error in the samples (main.go) that we need to ignore
//nolint:gocyclo could be scaffolded by default in the main,go
But if you have a better idea please feel free to try out
camilamacedo86
changed the title
.github/workflows/lint-sample.yml
Outdated
| @@ -11,6 +11,13 @@ on: | |||
| jobs: | |||
| lint-samples: | |||
There was a problem hiding this comment.
Could you rebase this one with master and ensure that here in this file the only change is to add a new test
- name: Check linter configuration
working-directory: ${{ matrix.folder }}
run: make lint-config
mateusoliveira43
force-pushed
the
fix/check-linter-configuration
branch
from
3592383 to
505b461
Compare
k8s-ci-robot
removed
the
needs-rebase
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
k8s-ci-robot
added
lgtm
|
see: https://github.com/kubernetes-sigs/kubebuilder/actions/runs/12546900344/job/34984003357?pr=4425 we need to fix the issue first /lgtm cancel |
k8s-ci-robot
removed
the
lgtm
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mateusoliveira43 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 |
k8s-ci-robot
removed
the
approved
Signed-off-by: Mateus Oliveira <[email protected]>
mateusoliveira43
force-pushed
the
fix/check-linter-configuration
branch
from
505b461 to
3446de3
Compare
|
@mateusoliveira43: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
| enable=fieldalignment: true | ||
| enable-all: true | ||
| disable: | ||
| - fieldalignment |
There was a problem hiding this comment.
@camilamacedo86 as configuration was wrong, it was not enforcing fieldalignment rule
I disabled it because there are several files affected
I suggest we merge this one, and fix the linter errors in follow up
Add check for linter configuration.
Fix #4416