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.

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

✨ (go/v4): Add new makefile target to check and validate the linter config #4425

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mateusoliveira43
Copy link
Contributor

@mateusoliveira43 mateusoliveira43 commented Dec 13, 2024

Add check for linter configuration.

Fix #4416

Signed-off-by: Mateus Oliveira <[email protected]>
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mateusoliveira43
Once this PR has been reviewed and has the lgtm label, please assign varshaprasad96 for approval. For more information see the Kubernetes Code Review Process.

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 13, 2024
@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 13, 2024
@k8s-ci-robot
Copy link
Contributor

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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 camilamacedo86 changed the title 🌱 fix: Check linter configuration ✨ : fix: Check linter configuration Dec 13, 2024
.PHONY: lint-config
lint-config: golangci-lint ## Verify golangci-lint linter configuration
$(GOLANGCI_LINT) config verify

Copy link
Member

Choose a reason for hiding this comment

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

We change things for end users, so as it is an addition, we should use ✨ since it should be highlighted in the release notes.

.golangci.yml Outdated
@@ -18,7 +18,7 @@ issues:

linters-settings:
govet:
enable=fieldalignment: true
Copy link
Member

@camilamacedo86 camilamacedo86 Dec 13, 2024

Choose a reason for hiding this comment

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

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

undid changes to configuration, just added new command to project Makefile, without calling it in CI

Copy link
Member

Choose a reason for hiding this comment

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

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?

with:
version: v1.59
working-directory: testdata/project-v4-with-plugins
args: --config .golangci.yml ./...
Copy link
Member

Choose a reason for hiding this comment

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

HI @mateusoliveira43

I gave an wrong direction here sorry that is 100% my fault
We will need

      - name: Run linter
        uses: golangci/golangci-lint-action@v6
        with:
          version: v1.59
          working-directory: testdata/project-v4-with-plugins
          args: --config .golangci.yml ./...

Otheriwse, if we broke the lint in the project that we scaffold we will not see GitHub comments

.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
Copy link
Member

@camilamacedo86 camilamacedo86 Dec 13, 2024

Choose a reason for hiding this comment

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

@mateusoliveira43

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 camilamacedo86 changed the title ✨ : fix: Check linter configuration ✨ (go/v4): Add new makefile target to check and validate the linter config Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new lint config verify command
3 participants