chore: Bump golangci-lint to v1.60.3 #3119
Conversation
gaby
requested review from
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThe changes consist of updates to GitHub Actions workflows, modifications to linter configurations, and refinements in test files across the codebase. Key updates include version increments for the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant GitHub Actions
participant Linter
participant Codebase
User->>GitHub Actions: Push changes
GitHub Actions->>Linter: Run linting process
Linter->>Codebase: Analyze code
Codebase->>Linter: Return results
Linter->>GitHub Actions: Send linting report
GitHub Actions->>User: Notify results
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3119 +/- ##
==========================================
- Coverage 80.03% 80.01% -0.02%
==========================================
Files 117 117
Lines 9035 9038 +3
==========================================
+ Hits 7231 7232 +1
- Misses 1373 1374 +1
- Partials 431 432 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
.github/workflows/markdown.yml (1)
Consider Reintroducing
pathsFilters in GitHub Actions WorkflowThe removal of the
pathsfilters in the GitHub Actions workflow will cause the markdownlint job to run on all changes to themasterormainbranches and for all pull requests, regardless of the file types involved. Given the presence of a significant number of non-Markdown files in the repository, this could lead to unnecessary workflow runs, potentially increasing the usage of GitHub Actions minutes. Consider reintroducing thepathsfilters to ensure that the workflow only runs when Markdown files are changed. Additionally, adding aworkflow_dispatchevent would allow for manual triggering of the workflow when needed, providing flexibility.
- File:
.github/workflows/markdown.yml- Suggested Configuration:
on: push: branches: - master - main paths: - "**/*.md" pull_request: paths: - "**/*.md" workflow_dispatch:Analysis chain
Line range hint
1-24: Verify the impact of removing thepathsfilters.Removing the
pathsfilters will trigger the workflow on any changes to themasterormainbranches forpushevents and for any pull requests, regardless of the file types involved. This might lead to unnecessary workflow runs for changes that don't involve Markdown files, potentially increasing the usage of GitHub Actions minutes.To verify the impact of this change, run the following script:
If the repository contains a significant number of non-Markdown files, consider keeping the
pathsfilters and adding anon.workflow_dispatchevent to allow manual triggering of the workflow when needed. This will ensure that the workflow runs only for relevant changes while providing flexibility to run it manually.on: push: branches: - master - main paths: - "**/*.md" pull_request: paths: - "**/*.md" workflow_dispatch:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the impact of removing the `paths` filters. # Test 1: Search for Markdown files in the repository. # Expect: Markdown files are present. fd --extension md # Test 2: Search for non-Markdown files in the repository. # Expect: Non-Markdown files are present. fd --exclude '*.md'Length of output: 7506
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (20)
- .github/workflows/linter.yml (1 hunks)
- .github/workflows/markdown.yml (1 hunks)
- .golangci.yml (4 hunks)
- Makefile (1 hunks)
- addon/retry/exponential_backoff_test.go (2 hunks)
- client/core_test.go (1 hunks)
- client/hooks_test.go (2 hunks)
- client/request_test.go (1 hunks)
- ctx_test.go (3 hunks)
- helpers.go (3 hunks)
- middleware/compress/compress_test.go (1 hunks)
- middleware/encryptcookie/encryptcookie_test.go (2 hunks)
- middleware/etag/etag.go (2 hunks)
- middleware/limiter/limiter_sliding.go (1 hunks)
- middleware/limiter/limiter_test.go (2 hunks)
- middleware/pprof/pprof_test.go (2 hunks)
- middleware/static/static_test.go (2 hunks)
- mount.go (2 hunks)
- mount_test.go (2 hunks)
- router.go (1 hunks)
Files skipped from review due to trivial changes (12)
- .github/workflows/linter.yml
- Makefile
- addon/retry/exponential_backoff_test.go
- client/hooks_test.go
- client/request_test.go
- ctx_test.go
- middleware/compress/compress_test.go
- middleware/encryptcookie/encryptcookie_test.go
- middleware/limiter/limiter_sliding.go
- middleware/limiter/limiter_test.go
- middleware/pprof/pprof_test.go
- middleware/static/static_test.go
Additional context used
GitHub Check: codecov/patch
middleware/etag/etag.go
[warning] 65-65: middleware/etag/etag.go#L65
Added line #L65 was not covered by tests
golangci-lint
mount.go
191-191: directive
//nolint:gosec // Not a concernis unused for linter "gosec"(nolintlint)
222-222: directive
//nolint:gosec // Not a concernis unused for linter "gosec"(nolintlint)
router.go
361-361: directive
//nolint:gosec // Not a concernis unused for linter "gosec"(nolintlint)
helpers.go
769-769: directive
//nolint:gosec // Casting in this function is not a concernis unused for linter "gosec"(nolintlint)
Additional comments not posted (12)
middleware/etag/etag.go (1)
60-62: LGTM!The code changes are approved.
client/core_test.go (1)
Line range hint
45-47: LGTM!The removal of the line that creates a new variable
ttfor use within the goroutine is a good change. It eliminates the shadowing of the loop variablett, which is a best practice to avoid potential data races when using loop variables in concurrent tests. The change improves clarity and reduces unnecessary variable declarations while maintaining the overall functionality of the test..golangci.yml (4)
281-281: Approved enabling thedupwordlinter.Enabling the
dupwordlinter is a good practice to detect duplicate words in the code.
290-290: Approved replacing theexportloopreflinter with thecopyloopvarlinter.The
exportloopreflinter is deprecated and replaced by thecopyloopvarlinter in newer versions ofgolangci-lint. Thecopyloopvarlinter detects loop variables that are copied by value instead of by reference, which can lead to unexpected behavior.
301-301: Approved enabling thegoconstlinter.Enabling the
goconstlinter is a good practice to detect repeated string constants in the code.
310-311: Please provide the reason for disabling thegomndlinter.The
gomndlinter detects magic numbers in the code. Disabling it might lead to magic numbers being introduced in the code. Please add a comment to document the reason for disabling the linter.router.go (1)
361-361: The added//nolint:gosec // Not a concerncomment is valid and correctly suppresses the "gosec" linter warning. The static analysis tool's hint about the unused directive seems to be a false positive in this case.Tools
golangci-lint
361-361: directive
//nolint:gosec // Not a concernis unused for linter "gosec"(nolintlint)
mount_test.go (2)
350-350: LGTM!The change to the custom error message is consistent with the test case and looks good.
397-397: LGTM!The assertion has been correctly updated to match the changed custom error message.
helpers.go (3)
329-329: LGTM!The change of
lastElementEndsAtvariable type fromuint8tointis approved. It allows the variable to store a broader range of values, which is relevant as the index can exceed the maximum value ofuint8in certain scenarios.
340-340: LGTM!The change in the assignment of
lastElementEndsAtis approved. It is consistent with the change in the type oflastElementEndsAtvariable fromuint8toint.
769-769: Keep the clarifying comment.The comment above the
genericParseTypefunction clarifies that the casting within this function is not a security concern. It is relevant and should be kept for future maintainers.The static analysis hint about the unused
//nolint:gosecdirective is a false positive and can be ignored.Tools
golangci-lint
769-769: directive
//nolint:gosec // Casting in this function is not a concernis unused for linter "gosec"(nolintlint)
| bodyLength := len(body) | ||
| if bodyLength > math.MaxUint32 { | ||
| return c.SendStatus(fiber.StatusRequestEntityTooLarge) | ||
| } | ||
|
|
||
| bb.B = appendUint(bb.Bytes(), uint32(bodyLength)) //nolint:gosec // Body length is validated above |
There was a problem hiding this comment.
LGTM!
The validation logic is correct and enhances the robustness of the ETag generation.
Add a test case for the added line.
The static analysis tool indicates that the added line #L65 is not covered by tests.
Do you want me to generate a test case for this line or open a GitHub issue to track this task?
Tools
GitHub Check: codecov/patch
[warning] 65-65: middleware/etag/etag.go#L65
Added line #L65 was not covered by tests
Description
golangci-lintto v1.60.3Etagmiddleware. Return error if greater thanMaxUint32Type of change