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

Configuration for golangci linter #4

Merged
merged 3 commits into from
Apr 12, 2023
Merged

Conversation

mvshao
Copy link
Contributor

@mvshao mvshao commented Apr 5, 2023

Description

Changes proposed in this pull request:

  • add configuration file for golangci linter

Related PR
test-infra

@mvshao mvshao requested a review from a team as a code owner April 5, 2023 12:56
@kyma-bot kyma-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 5, 2023
@kyma-bot kyma-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 5, 2023
@VOID404
Copy link
Contributor

VOID404 commented Apr 6, 2023

could you link a relevant test-infra PR? Other than that, /lgtm
/hold

@kyma-bot kyma-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2023
VOID404
VOID404 previously approved these changes Apr 6, 2023
@kyma-bot
Copy link
Contributor

kyma-bot commented Apr 6, 2023

@mvshao: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
mvshao_test_of_prowjob_pre-main-compass-manager-lint a522d92 link false /test pre-main-compass-manager-lint

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

@kyma-bot kyma-bot removed the lgtm Looks good to me! label Apr 6, 2023
@@ -38,6 +38,14 @@ help: ## Display this help.

##@ Development

lint-autofix: ## Autofix all possible linting errors.
golangci-lint run -E goimports --fix
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't we reuse the .golangci.yaml for fixing? ie instead of -E, use the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, we need to run only one linter, specifically goimports, which is able to fix errors related to incorrect formatting of imports. Running all the linter does not seem to be optimal in my opinion. This target in the Makefile is supposed to be used, by default, only to fix incorrectly formatted imports.

Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't we also fix other lints that do support fixing?
Is this only imports? Probably general formatting could also be done this way, possibly whitespace changes (if we have that lint enabled) etc

Could we do something like "all from config, except non-fast"? IIRC golangci-lint had some tag for fast lints?

Copy link
Contributor Author

@mvshao mvshao Apr 12, 2023

Choose a reason for hiding this comment

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

For the moment, we just need the goimports fix to be available. Most of the errors that can happen are errors regarding imports. If we see that adding more linters will help solve problems that may arise during further implementation, we will do so in the next iteration. Unfortunately, I could not find in the golangci documentation, a flag that describes a linter as fast.
I'm also not comfortable with the idea of running all linter with the --fix parameter, due to their significant number.

@kyma-bot kyma-bot added the lgtm Looks good to me! label Apr 12, 2023
@mvshao mvshao removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 12, 2023
@kyma-bot kyma-bot merged commit cb4c80f into kyma-project:main Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Looks good to me! size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants