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

chore(linter): update Golang linter configuration #1977

Merged
merged 21 commits into from
Jul 31, 2024
Merged

Conversation

sesheffield
Copy link
Contributor

@sesheffield sesheffield commented Jul 25, 2024

Description

  • adding a .golangci.yml file to dictate the golang linters.
    • disabling any that are currently failing to prevent master from failing
  • added CODEOWNERS

To test locally:

  • act -j golangci-lint

Strategy moving forward:

  • Team has decided to use new-from-rev that runs the linter against the files changed in a PR.
    • --new-from-rev REV Show only new issues created after git revision REV
  • We should move forward enabling linters by uncommenting in: .golangci.yml
  • Fixing linters should be continually addressed as normal tech debt and apart of the typical development cycle.

Checklist

  • Changelog has been updated as necessary.

@sesheffield sesheffield requested review from drklee3, pirtleshell and karzak and removed request for drklee3 July 25, 2024 20:44
Copy link
Member

@pirtleshell pirtleshell left a comment

Choose a reason for hiding this comment

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

i appreciate your changes here. however, i think this PR is doing too much at once and should be broken into more singularly-focused pieces.

additionally your commit messages and PR descriptions should include details on why changes are being made and be more specific. for example "added a new make command", does not tell a reader of the git log what was actually changed. PR titles & commit messages should follow the guidelines outlined in https://www.conventionalcommits.org/en/v1.0.0/. Example: "feat(ci): enabled golang linters" (assuming this is the only thing the PR does, which is not the case for this PR).

regarding the linters: i think we need to first decide as a team what lints to use. i also think the final form of linting should prefer starting with the defaults and only adding or disabling functionality as the need arises, rather than starting with everything disabled and enabling individual linters/rules. this will also make the config file much simpler and easier to read. exceptions to the defaults, including additional new linters, should have explanatory comments in the config YAML.

some other thoughts/questions:

  • please also delete the existing, incorrectly named config file
  • please add @faddat as a co-author to the commit that changes the config file. he previously did a good amount of prep for the linters, including identifying the misnamed file in Linters haven't been run recently #1636 (thank you, @faddat!!)
  • does the current codebase pass all these lints? are we sure the next commit to master won't fail CI because of things enabled here?

.github/workflows/ci-lint.yml Outdated Show resolved Hide resolved
.golangci-version Outdated Show resolved Hide resolved
.github/workflows/ci-lint.yml Outdated Show resolved Hide resolved
.github/CODEOWNERS Show resolved Hide resolved
@sesheffield
Copy link
Contributor Author

sesheffield commented Jul 26, 2024

i appreciate your changes here. however, i think this PR is doing too much at once and should be broken into more singularly-focused pieces.

additionally your commit messages and PR descriptions should include details on why changes are being made and be more specific. for example "added a new make command", does not tell a reader of the git log what was actually changed. PR titles & commit messages should follow the guidelines outlined in https://www.conventionalcommits.org/en/v1.0.0/. Example: "feat(ci): enabled golang linters" (assuming this is the only thing the PR does, which is not the case for this PR).

regarding the linters: i think we need to first decide as a team what lints to use. i also think the final form of linting should prefer starting with the defaults and only adding or disabling functionality as the need arises, rather than starting with everything disabled and enabling individual linters/rules. this will also make the config file much simpler and easier to read. exceptions to the defaults, including additional new linters, should have explanatory comments in the config YAML.

some other thoughts/questions:

  • please also delete the existing, incorrectly named config file
  • please add @faddat as a co-author to the commit that changes the config file. he previously did a good amount of prep for the linters, including identifying the misnamed file in Linters haven't been run recently #1636 (thank you, @faddat!!)
  • does the current codebase pass all these lints? are we sure the next commit to master won't fail CI because of things enabled here?
  1. please also delete the existing, incorrectly named config file
    • not sure what file you're referring to.
  2. does the current codebase pass all these lints? are we sure the next commit to master won't fail CI because of things enabled here?

@sesheffield sesheffield changed the title Golang linter chore(linter): update Golang linter configuration Jul 29, 2024
@sesheffield
Copy link
Contributor Author

@pirtleshell

regarding the linters: i think we need to first decide as a team what lints to use. i also think the final form of linting should prefer starting with the defaults and only adding or disabling functionality as the need arises, rather than starting with everything disabled and enabling individual linters/rules. this will also make the config file much simpler and easier to read. exceptions to the defaults, including additional new linters, should have explanatory comments in the config YAML.

I agree that we should discuss and finalize the linters as a team. I can set up a time with the team to discuss this further. My approach was to add all linters initially, as I found that even the basic linters were failing, which made starting with a comprehensive setup more practical.

The linters currently enabled and disabled in the configuration align with the existing "style" the team has developed. Starting with all linters that we can have enabled allows us to identify and address any issues early on. Once we have a baseline where everything is passing, we can then adjust the configuration by enabling or disabling specific rules as needed. This approach will help us ensure that we maintain a high standard of code quality from the start.

I agree that the final configuration should be simplified and commented for clarity. Once we finalize the linters and rules, I'll make sure to document any exceptions and customizations in the config YAML.

Let me know if you have any specific linters or rules you'd like to prioritize, and we can adjust accordingly.

.golangci.yml Outdated
for-loops: false # disables preallocation checks in for loops

issues:
exclude-rules:
Copy link
Member

Choose a reason for hiding this comment

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

can we remove the exclusions until they rear their ugly heads in future commits?

Copy link
Member

@pirtleshell pirtleshell left a comment

Choose a reason for hiding this comment

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

looks great, thanks for the much needed addition!

@sesheffield sesheffield merged commit b0d737d into master Jul 31, 2024
11 checks passed
@sesheffield sesheffield deleted the golang-linter branch July 31, 2024 20:23
pirtleshell pushed a commit that referenced this pull request Oct 25, 2024
* update golinter + add go sec

* add golangci.yml
Co-authored-by: @faddat [email protected]

* update

* update

* fix release version

* remove sec, update from pr comments, cleanup golangci.yml to not break on master

* remove @faddat, not valid codeowner

* remove unnecessary make command

* remove incorrectly named golangci.yml file

* add --new-from-rev

* use master instead of main

* remove extra echo

* set the exports properly

* add setup go to work with act

* add some docs to golangci linter

* test new-from-rev

* enable more linters, but app.go back

* verify issues-exit-code being gone

* put it back

* enable more linters

* remove exclusions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants