feat: make golangci-lint config stricter #2874
Conversation
leonklingele
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
|
@nickajacks1 can you verify these changes |
There was a problem hiding this comment.
Thanks for the super thorough update. Left a lot of comments (sorry). Some comments are just me thinking out loud so feel free to resolve those right away.
There were a number of linters that are either deprecated or audit usage of packages that we don't use. I recommend explicitly disabling all of them. For contrib we can enable ones like spancheck and zerologlint.
| # - wastedassign # TODO https://github.com/gofiber/fiber/issues/2816 | ||
| - varcheck | ||
| # - varnamelen | ||
| # - wastedassign # TODO: Enable |
There was a problem hiding this comment.
I looked into this a bit and wastedassign feels like it is doing exactly what ineffassign does, except that it errs on the side of false positives instead of false negatives. The only failures in fiber today triggered by wastedassign are zero-initialization (eg foo := "" instead of var foo string)
So I might put my vote on disabling wastedassign. Dont need to decide that now though.
There was a problem hiding this comment.
I'd prefer consistency here. For example, we use both var foo string and foo := "" in this codebase.
| - whitespace | ||
| - wrapcheck | ||
| - tenv | ||
| # - wsl |
There was a problem hiding this comment.
I believe wsl directly conflicts with gofumpt in some areas, so it is impossible to enable both with default configuration. We can probably add config to make them agree though.
.golangci.yml
Outdated
| - hugeParam | ||
| - rangeExprCopy | ||
| - rangeValCopy |
There was a problem hiding this comment.
does the above TODO also apply to these
There was a problem hiding this comment.
No, it's an inline-comment, so it only applies that that one specified line.
|
|
||
| gofumpt: | ||
| module-path: github.com/gofiber/fiber | ||
| extra-rules: true | ||
|
|
||
| gosec: | ||
| excludes: | ||
| - G104 # Provided by errcheck | ||
| - G104 # TODO: Enable this again. Mostly provided by errcheck |
There was a problem hiding this comment.
Do you know which cases are not handled by errcheck?
There was a problem hiding this comment.
I don't know off-head right now and don't have access to a Go system at the moment. Simply try it yourself and drop the line :D
There was a problem hiding this comment.
In the current codebase it doesn't catch anything that other linters don't catch, but I'd like to see some cases that it does catch which the others dont.
Actually, I just remembered another reason why I disabled it - golangci-lint has a bug that prevents you from setting global ignores for this rule golangci/golangci-lint#3749
So until that get's fixed, you'd have to manually add ignores everywhere, which adds a lot of noise to the code.
| loggercheck: | ||
| require-string-key: true | ||
| no-printf-like: true |
There was a problem hiding this comment.
I think so, see their examples at timonwong/loggercheck@ef578c4#diff-dbd79b07e7f11adb8139d961bff8a7df5f2e3f331e92e9d13d8f34ff84595cdeR1
|
I think it's fine if we leave enable ones that don't apply. That way we don't have to be tailoring the config for each repo. The golangci-lint job is quite quick compared to the Tests and the Benchmarks. |
|
I agree that having a common config for all the projects makes sense, so enabling things like spancheck probably makes things easier. |
WalkthroughThe recent updates encompass a range of improvements across configuration, middleware logic, and internal function optimizations. Key adjustments include enhanced timeout handling and runner settings in the linter configuration, refined idempotency handling in HTTP requests, a more resilient session store operation, and smarter path processing logic. Additionally, flash message parsing in redirects has been fine-tuned for accuracy. Collectively, these changes aim to boost code quality, performance, and maintainability. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 8
Configuration used: CodeRabbit UI
Files selected for processing (5)
- .golangci.yml (6 hunks)
- middleware/idempotency/idempotency_test.go (1 hunks)
- middleware/session/store.go (1 hunks)
- path.go (1 hunks)
- redirect.go (1 hunks)
Additional comments: 7
middleware/idempotency/idempotency_test.go (1)
- 47-48: The adjustments made to the logic for handling idempotency in HTTP requests when the X-Idempotency-Key header is not set improve clarity and accuracy. Using clear and specific error messages in tests is a good practice, as it helps identify the reason for test failures more easily.
redirect.go (1)
- 281-286: The adjustments made to the logic for parsing and setting flash messages from a cookie, specifically to ensure the last message is handled correctly, are well thought out. Correctly handling the last flash message is crucial for user feedback mechanisms, and this change addresses a potential issue with the previous logic. Good job on improving the robustness of the flash message parsing logic.
.golangci.yml (4)
- 13-13: The
outputsettinguniq-by-lineis set tofalse. This change might affect the readability and manageability of linting results, especially in cases where multiple issues are reported on the same line. It's worth considering the impact on the development workflow and whether this setting aligns with the team's preferences for handling lint results.Consider discussing with the team to ensure this setting aligns with how you prefer to review and manage linting results.
- 32-39: The
errchecklinter settings have been adjusted to include checks for type assertions and blank identifiers, and to disable default exclusions. These changes enhance the thoroughness of error checking. However, specific functions are excluded from checks, which is reasonable but requires justification to ensure that these exclusions do not overlook potential errors.Ensure that the rationale for excluding specific functions from
errcheckis documented, either in the configuration file or related documentation, to maintain clarity on why these exceptions are made.
- 45-45: The
exhaustivelinter is configured to check generated files and treat missing cases in switch statements over enums as exhaustive by default. This is a good practice for ensuring completeness in handling enum cases. However, it's important to assess the impact on generated code and ensure that this setting does not introduce unnecessary burdens on developers.Evaluate the impact of enabling
check-generatedanddefault-signifies-exhaustiveon the development workflow, especially regarding generated code, to ensure it aligns with project standards.
- 258-266: The
issuesconfiguration has been updated with new settings, includingexclude-use-default,exclude-case-sensitive, and rules for maximum issues per linter and same issues. Additionally, specific rules are defined to exclude certain texts. These changes are aimed at refining the handling of issues reported by linters. However, it's important to ensure that these settings do not inadvertently suppress important feedback from the linters.Review the
issuesconfiguration to ensure that it effectively balances between reducing noise and not missing critical issues. Consider the impact of these settings on the visibility of important linter feedback.path.go (1)
- 594-599: The refactoring in the
findGreedyParamLenfunction introduces a more efficient control flow by breaking the loop early ifconstPositionis-1. This change is logical and improves the performance by avoiding unnecessary iterations over the stringswhen thesegment.ComparePartis not found anymore. This is a good practice in optimizing loop operations, especially in string manipulation tasks which can be performance-sensitive.However, it's important to ensure that this change does not alter the expected behavior of the function, especially in edge cases where
segment.ComparePartmight be an empty string or whensearchCountis initially0. Adding unit tests or enhancing existing ones to cover these scenarios would be beneficial to ensure the robustness of this change.
| @@ -75,7 +75,7 @@ func (s *Store) Get(c fiber.Ctx) (*Session, error) { | |||
| if raw != nil && err == nil { | |||
| mux.Lock() | |||
| defer mux.Unlock() | |||
| sess.byteBuffer.Write(raw) | |||
| _, _ = sess.byteBuffer.Write(raw) // Ignore error, this will never fail | |||
There was a problem hiding this comment.
Ignoring the error returned by sess.byteBuffer.Write(raw) is risky, even if it's assumed that this operation will never fail. It's generally a good practice to handle or at least log errors to catch unexpected scenarios, especially in I/O operations. Can you please verify this assumption in the broader context of how sess.byteBuffer is used throughout the application? Additionally, considering the use of a mutex around this operation, ensure that the concurrency model doesn't introduce edge cases where the write operation might fail.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Configuration used: CodeRabbit UI
Files selected for processing (2)
- .golangci.yml (7 hunks)
- middleware/idempotency/idempotency_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- middleware/idempotency/idempotency_test.go
Additional comments: 3
.golangci.yml (3)
- 13-13: The
uniq-by-linesetting in theoutputsection has been adjusted tofalse. This change affects how linting results are aggregated and displayed. Ensure that this setting aligns with the team's preferences for reviewing linting results, as it could potentially lead to more verbose output by not uniquing results by line.- 32-39: The
errchecklinter configuration has been updated to include checks for type assertions and blank identifiers, and to disable default exclusions. Additionally, specific functions have been excluded from checks. This configuration enhances error handling checks but requires careful consideration of the excluded functions to ensure important errors are not missed. Verify that the chosen exclusions are appropriate and do not overlook critical error checks.- 258-266: The
issuesconfiguration has been updated with new settings, includingexclude-use-default,exclude-case-sensitive,max-issues-per-linter, andmax-same-issues. Additionally, specific rules have been defined to exclude certain linters or texts. It's important to ensure that these configurations align with the project's goals for code quality and do not inadvertently ignore significant issues. Review the exclusions and limits set to confirm they are appropriate and do not compromise the effectiveness of the linting process.
.golangci.yml
Outdated
| depguard: | ||
| rules: | ||
| all: | ||
| list-mode: lax | ||
| deny: | ||
| - pkg: "flag" | ||
| desc: '`flag` package is only allowed in main.go' | ||
| - pkg: "io/ioutil" | ||
| desc: '`io/ioutil` package is deprecated, use the `io` and `os` package instead' | ||
| # TODO: Prevent using these without a reason | ||
| # - pkg: "reflect" | ||
| # desc: '`reflect` package is dangerous to use' | ||
| # - pkg: "unsafe" | ||
| # desc: '`unsafe` package is dangerous to use' |
There was a problem hiding this comment.
The depguard linter settings have been refined, including specifying packages to deny with descriptive reasons. This is a good practice for maintaining code quality and preventing the use of deprecated or undesirable packages. However, there are commented-out entries for the reflect and unsafe packages with a TODO note suggesting a need for justification when using these packages. It's important to decide whether to enforce these rules or not, as leaving commented-out code can lead to confusion.
Consider finalizing the decision on the reflect and unsafe packages and either enforce the rules or remove the commented-out code to avoid confusion.
| - ^print(ln)?$ | ||
| - ^fmt\.Print(f|ln)?$ | ||
| - ^http\.Default(Client|ServeMux|Transport)$ | ||
| # TODO: Eventually enable these patterns | ||
| # - 'time\.Sleep' | ||
| # - 'panic' | ||
| # - ^panic$ | ||
| # - ^time\.Sleep$ |
There was a problem hiding this comment.
The forbidigo linter settings include forbidding certain patterns like print, fmt.Print, and http.Default* with a TODO note to eventually enable more patterns. This approach helps maintain code quality by discouraging the use of certain functions. However, the commented-out patterns for panic and time.Sleep suggest an intention to further refine the rules, which should be addressed to avoid leaving unfinished configurations.
Finalize the decision on forbidding additional patterns like panic and time.Sleep to ensure the linter configuration is complete and reflects the project's coding standards.
| gci: | ||
| sections: | ||
| - standard | ||
| - prefix(github.com/gofiber/fiber) | ||
| - default | ||
| - blank | ||
| - dot | ||
| # - alias | ||
| custom-order: true | ||
|
|
There was a problem hiding this comment.
The gci linter settings define a custom order for import sections, which is a good practice for maintaining consistency across the codebase. However, the alias section is commented out, suggesting uncertainty about its inclusion. It's important to clarify the intended import order and ensure it's consistently applied across the project.
Decide on the inclusion of the alias section in the import order and update the configuration accordingly to avoid ambiguity and ensure consistent import formatting.
| # TODO: Uncomment the following lines | ||
| # enabled-tags: | ||
| # - diagnostic | ||
| # - style | ||
| # - performance | ||
| # - experimental | ||
| # - opinionated |
There was a problem hiding this comment.
The gocritic linter has several TODO comments related to enabling tags and disabling checks. While it's beneficial to customize the linter for specific needs, leaving TODO comments without addressing them can lead to confusion and an incomplete configuration.
Address the TODO comments in the gocritic linter settings by finalizing the decisions on enabled tags and disabled checks to ensure the configuration is clear and complete.
|
|
||
| gofumpt: | ||
| module-path: github.com/gofiber/fiber | ||
| extra-rules: true | ||
|
|
||
| gosec: | ||
| excludes: | ||
| - G104 # Provided by errcheck | ||
| - G104 # TODO: Enable this again. Mostly provided by errcheck |
There was a problem hiding this comment.
The gosec linter excludes the G104 check with a TODO note to enable it again. This exclusion might overlook unchecked errors, which can lead to potential security issues. It's crucial to assess the impact of this exclusion and consider enabling the G104 check to ensure comprehensive security analysis.
Re-evaluate the decision to exclude the G104 check from the gosec linter and consider enabling it to ensure thorough security analysis and maintain high code quality.
.golangci.yml
Outdated
| - exportloopref | ||
| - forbidigo | ||
| - forcetypeassert | ||
| # - gochecksumtype # TODO https://github.com/gofiber/fiber/issues/2816 | ||
| # - goconst # TODO https://github.com/gofiber/fiber/issues/2816 | ||
| # - funlen | ||
| # - gci # TODO: Enable | ||
| - ginkgolinter | ||
| # - gocheckcompilerdirectives # TODO: Enable | ||
| # - gochecknoglobals # TODO: Enable | ||
| # - gochecknoinits # TODO: Enable | ||
| - gochecksumtype | ||
| # - gocognit | ||
| # - goconst # TODO: Enable | ||
| - gocritic | ||
| # - gocyclo | ||
| # - godot | ||
| # - godox | ||
| - goerr113 | ||
| - gofmt | ||
| - gofumpt | ||
| - goimports | ||
| # - goheader | ||
| # - goimports | ||
| # - golint | ||
| # - gomnd # TODO: Enable | ||
| - gomoddirectives | ||
| # - gomodguard | ||
| - goprintffuncname | ||
| - gosec | ||
| - gosimple | ||
| # - gosmopolitan # TODO https://github.com/gofiber/fiber/issues/2816 | ||
| # - gosmopolitan # TODO: Enable | ||
| - govet | ||
| - grouper | ||
| - inamedparam | ||
| # - ifshort # TODO: Enable | ||
| # - importas | ||
| # - inamedparam | ||
| - ineffassign | ||
| # - interfacebloat | ||
| # - interfacer | ||
| # - ireturn | ||
| # - lll | ||
| - loggercheck | ||
| # - maintidx | ||
| - makezero | ||
| # - maligned | ||
| - mirror | ||
| - misspell | ||
| - musttag | ||
| - nakedret | ||
| # - nestif | ||
| - nilerr | ||
| - nilnil | ||
| # - nlreturn | ||
| - noctx | ||
| - nolintlint | ||
| - nonamedreturns | ||
| - nosnakecase | ||
| - nosprintfhostport | ||
| # - paralleltest # TODO: Enable | ||
| - perfsprint | ||
| # - prealloc | ||
| - predeclared | ||
| - promlinter | ||
| - protogetter | ||
| - reassign | ||
| - revive | ||
| - rowserrcheck | ||
| # - scopelint # TODO: Enable | ||
| - sloglint | ||
| - spancheck | ||
| - sqlclosecheck | ||
| - staticcheck | ||
| - structcheck | ||
| - stylecheck | ||
| # - tagalign # TODO: Enable | ||
| - tagliatelle | ||
| - tenv | ||
| - testableexamples | ||
| - testifylint | ||
| # - testpackage # TODO: Enable once https://github.com/gofiber/fiber/issues/2252 is implemented | ||
| # - testpackage # TODO: Enable | ||
| - thelper | ||
| - tparallel | ||
| - typecheck | ||
| - unconvert | ||
| - unparam | ||
| - unused | ||
| - usestdlibvars | ||
| # - wastedassign # TODO https://github.com/gofiber/fiber/issues/2816 | ||
| - varcheck | ||
| # - varnamelen | ||
| # - wastedassign # TODO: Enable | ||
| - whitespace | ||
| - wrapcheck | ||
| - tenv | ||
| # - wsl | ||
| - zerologlint |
There was a problem hiding this comment.
The linters section has been extensively modified, including enabling and disabling various linters. Some linters are marked as deprecated in favor of others, and there are several TODO comments suggesting further actions. It's essential to finalize decisions on deprecated linters and address TODO comments to ensure the linter configuration is up-to-date and reflects the project's standards.
Finalize decisions on deprecated linters and address TODO comments in the linters section to ensure the configuration is clear, complete, and aligned with best practices.
|
What's blocking this? |
|
@nickajacks1 Has everything been clarified? or are there still open points? |
|
@ReneWerner87 @leonklingele I don't think we should enable deprecated linters, but my opinion shouldn't block this being merged. Also, inamedparam was disabled here, I assume by accident. |
|
Yes, pls don't activate deprecated linters |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- middleware/idempotency/idempotency_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- middleware/idempotency/idempotency_test.go
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2874 +/- ##
==========================================
- Coverage 82.73% 82.65% -0.08%
==========================================
Files 116 116
Lines 8369 8367 -2
==========================================
- Hits 6924 6916 -8
- Misses 1103 1107 +4
- Partials 342 344 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@leonklingele The merge from main broke the golangci-lint yaml. There's a duplicated key |
|
@leonklingele friendly ping |
|
Sorry for the long delay — once again! I don't have a dev system at hand but will be able to update this until end of the week. |
leonklingele
force-pushed
the
make-golangci-lint-config-stricter
branch
from
c215d93 to
63cc8c7
Compare
|
Rebased & removed deprecated linters. PTAL :) |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files selected for processing (7)
- .golangci.yml (6 hunks)
- listen_test.go (1 hunks)
- middleware/cors/cors.go (1 hunks)
- middleware/idempotency/idempotency_test.go (1 hunks)
- middleware/session/store.go (1 hunks)
- path.go (1 hunks)
- redirect.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- middleware/idempotency/idempotency_test.go
- middleware/session/store.go
- path.go
- redirect.go
Additional comments: 1
middleware/cors/cors.go (1)
- 129-132: The logic for handling invalid origins in the CORS configuration has been adjusted to log an error and continue processing, ensuring that only valid origins are stored in
allowOrigins. This change improves the robustness of the CORS middleware by preventing invalid origins from being mistakenly allowed.
| timeout: 5m | ||
| skip-dirs-use-default: false | ||
| modules-download-mode: readonly | ||
| allow-serial-runners: true | ||
| skip-dirs: | ||
| - internal | ||
| - internal # TODO: Do not ignore interal |
There was a problem hiding this comment.
The run section has been updated with new settings including timeout, allow-serial-runners, and a modification to skip-dirs. The TODO comment on line 9 suggests an intention to revisit the decision to ignore the internal directory. It's important to ensure that this TODO is tracked and addressed to avoid unintentionally skipping important directories in the linting process.
Consider creating a tracking issue or plan to address the TODO regarding the internal directory to ensure it does not get overlooked.
| time-layout: false # TODO: Set to true | ||
| crypto-hash: true | ||
| default-rpc-path: true | ||
| os-dev-null: true |
There was a problem hiding this comment.
MinVersion is missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported and insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. Add MinVersion: tls.VersionTLS13 to the TLS configuration to bump the minimum version to TLS 1.3.
- config := &tls.Config{Certificates: []tls.Certificate{cer}}
+ config := &tls.Config{Certificates: []tls.Certificate{cer}, MinVersion: tls.VersionTLS13}Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| os-dev-null: true |
| os-dev-null: true | ||
| sql-isolation-level: true | ||
| tls-signature-scheme: true | ||
| constant-kind: true |
There was a problem hiding this comment.
Detected a network listener listening on 0.0.0.0 or an empty string. This could unexpectedly expose the server publicly as it binds to all available interfaces. Instead, specify another IP address that is not 0.0.0.0 nor the empty string.
Consider specifying a more restrictive IP address to bind the server to, enhancing security by limiting the network interfaces the server is exposed on.
| @@ -8,7 +8,7 @@ import ( | |||
| "errors" | |||
| "fmt" | |||
| "io" | |||
| "log" | |||
| "log" //nolint:depguard // TODO: Required to capture output, use internal log package instead | |||
There was a problem hiding this comment.
The addition of a comment regarding the use of the log package suggests the need to capture output and hints at a future change to use an internal log package instead. This is a good practice to ensure that logging is consistent and centralized within the project. Tracking this TODO for future implementation is recommended.
Would you like me to help with implementing the internal log package or create a tracking issue for this task?
Summary by CodeRabbit