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

[MB-13510] Add gci linter to make go import order/grouping more consistent #9064

Merged
merged 5 commits into from
Aug 29, 2022

Conversation

reggieriser
Copy link
Contributor

@reggieriser reggieriser commented Aug 15, 2022

Jira ticket for this change

Summary

I've noticed that the order of our go imports has seemed a little random and getting more so over time. Ideally, the imports would be organized as follows (alphabetically within a section):

import (
    [system packages]

    [third-party packages]

    [project packages]
)

However, when a blank line is added between imports, goimports won't regroup the grouped imports to be with other similar groups. In this PR, I used the gci linter via golangci-lint to do the reorganization which eliminates extra sections and groups like I think we want.

A lot of files have changed, but you should be able to follow the commits as so:

  1. Update all pre-commit hooks; fix issues discovered by updates: this updates all of our pre-commit hooks (via pre-commit autoupdate) to their latest versions, then fixes a few new issues that popped up from the updated versions of golangci-lint and markdownlint. I needed the latest golangci-lint for gci to work properly.
  2. Update nix config to use pre-commit 2.20: don't think that was required, but I noticed the nix config was several versions behind a brew'd version as well as what we use in the circleci-docker repository.
  3. Removed some import duplication that was causing inconsistent import formatting by linter (and just confusing): fixed some import duplication and import comments that were causing gci to flip import order back and forth when run. I assume it was unintentional (although legal) to have the import duplication.
  4. Add gci linter to golangci-lint to format order/grouping of go imports: added the gci linter and settings to golangci-lint and included the go files where gci fixed the import order (appears to be most of them).

To test it, try changing the order of imports on a file or two, do a git add on them, then run:
pre-commit run golangci-lint

To try it on all files (it shouldn't touch generated files),
pre-commit run -a golangci-lint

Special Notes

  • one of the new linter issues I had to fix involved setting a ReadHeaderTimeout in addition to the existing IdleTimeout (gosec rule G112: Potential slowloris attack). Since it wasn't set previously, I think there was no timeout. I set it to 60 seconds (half of the 120 second idle timeout), but I could use some advice on if another value makes more sense.
  • there is a performance problem when running the revive linter under golangci-lint in the latest 1.47.*+ versions. Temporarily, golangci-lint has turned off some revive rules as a workaround until it can be fixed upstream. I'm hoping this might be fixed by the time we would consider merging this, so I'm making this a draft PR for now. If not, we'll have to assess if it makes sense to upgrade now and have some rules turned off for a bit.
  • the performance problem noted above has been fixed in revive and included in golangci-lint 1.49.0 (along with reversing the rules that had been temporarily turned off).
  • the latest golangci-lint has deprecated a few linters that we use. Rather than complicate this PR any further, I'd prefer to assess the impact of those deprecations in a separate PR. We can also review our use of this workaround as well since some linters are more generics-friendly now.

Handler: input.HTTPHandler,
IdleTimeout: idleTimeout,
MaxHeaderBytes: maxHeaderSize,
ReadHeaderTimeout: readHeaderTimeout,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Original linter issue: G112: Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server (gosec)

@@ -60,7 +60,6 @@ func CreatePrimeClientWithCACStoreParam(v *viper.Viper, store *pksigner.Store) (
MinVersion: tls.VersionTLS12,
MaxVersion: tls.VersionTLS12,
}
tlsConfig.BuildNameToCertificate()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Original linter issue (several of these): SA1019: tlsConfig.BuildNameToCertificate has been deprecated since Go 1.14: NameToCertificate only allows associating a single certificate with a given name. Leave that field nil to let the library select the first compatible chain from Certificates. (staticcheck)

Comment on lines +50 to +53
[contributors]: https://github.com/transcom/mymove/blob/master/CONTRIBUTORS.md
[issues]: https://github.com/transcom/mymove/issues
[license]: https://github.com/transcom/mymove/blob/master/LICENSE.txt
[pulls]: https://github.com/transcom/mymove/pulls
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Example original linter issue (several in this file): MD053/link-image-reference-definitions Link and image reference definitions should be needed [Unused link or image reference definition: "contributors"] [Context: "[contributors]: https://github..."]

@reggieriser reggieriser added the wip Work In Progress label Aug 15, 2022
@robot-mymove
Copy link

robot-mymove commented Aug 15, 2022

Messages
📖 🔗 MB-13510

Generated by 🚫 dangerJS against ee6277d

@reggieriser reggieriser force-pushed the rr-MB-13510-add-gci-linter branch 3 times, most recently from 619e757 to 2893865 Compare August 22, 2022 13:50
@reggieriser reggieriser marked this pull request as ready for review August 24, 2022 14:01
@reggieriser reggieriser requested review from a team August 24, 2022 14:01
@reggieriser reggieriser added ttv and removed wip Work In Progress labels Aug 24, 2022
Copy link
Contributor

@LeDeep LeDeep left a comment

Choose a reason for hiding this comment

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

Played around with order of imports and pre-commit run -a golangci-lint was able to resolve them. Changes looks good. Updated links are all working. Tests pass and was able to run server without issue.

@mergify mergify bot merged commit e2f732f into master Aug 29, 2022
@mergify mergify bot deleted the rr-MB-13510-add-gci-linter branch August 29, 2022 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants