chore: upgrade Go 1.15 to 1.25, add CI linting, fix lint issues#8
chore: upgrade Go 1.15 to 1.25, add CI linting, fix lint issues#8
Conversation
- Bump go.mod from Go 1.15 to 1.25.0 - Update all dependencies to latest versions - Fix fmt.Errorf vet issue (use errors.New for non-format strings) - Fix unchecked raven.SetDSN error, merge var decl, use switch - Add golangci-lint config and CI lint job - Add -race flag to test target - Standardize CI with separate build/test/lint jobs
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughUpdated Go toolchain from 1.15 to 1.25.0 and bumped multiple dependencies including gRPC and uuid packages. Added golangci-lint configuration and enhanced CI workflows with newer GitHub Actions versions and dedicated test and lint job definitions. Expanded Makefile with race-enabled testing, linting, and benchmark targets. Refactored error handling and notification logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR modernizes the module’s Go toolchain and dependency set, adds standardized CI linting, and applies small code changes to satisfy vet/lint requirements.
Changes:
- Upgraded
go.modGo version and refreshed dependencies. - Added golangci-lint configuration and a dedicated CI lint job.
- Fixed lints/vet issues in notifier + error construction, and updated Makefile targets (including
-race).
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
notifier/notifier.go |
Handles raven.SetDSN error, cleans up notice var init, uses severity switch, and errcheck-friendly goroutine call. |
go.mod |
Bumps Go version and updates direct/indirect dependencies. |
errors.go |
Replaces fmt.Errorf(msg) with errors.New(msg) to satisfy vet on non-format strings. |
Makefile |
Adds lint/bench targets and updates test to run with race detector. |
.golangci.yml |
Introduces repository golangci-lint configuration. |
.github/workflows/go.yml |
Splits CI into build/test/lint jobs; updates actions and pins Go version. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/go.yml (1)
6-16:⚠️ Potential issue | 🟠 MajorSet explicit least-privilege workflow permissions.
The workflow currently relies on default
GITHUB_TOKENpermissions. For these read-only CI tasks, declare minimal permissions explicitly.Proposed fix
on: push: branches: [ "main" ] pull_request: branches: [ "main" ] + +permissions: + contents: read🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/go.yml around lines 6 - 16, The workflow relies on default GITHUB_TOKEN permissions; add an explicit least-privilege permissions block at the top-level of the workflow (above jobs) to restrict token scopes for the CI run. Add a permissions map (e.g., permissions: contents: read, actions: read, packages: read) to limit access for the GITHUB_TOKEN used by the build job and other steps; ensure the block is applied at the workflow root (not inside job 'build') so all steps use the reduced scopes.
🧹 Nitpick comments (2)
notifier/notifier.go (1)
327-327: Avoid silently dropping async notify failures.Line [327] discards
Notifyerrors. Logging failures inside the goroutine improves incident visibility without changing control flow.Proposed fix
-go func() { _ = Notify(err, list...) }() +go func() { + if notifyErr := Notify(err, list...); notifyErr != nil { + log.Error(context.Background(), "msg", "async notify failed", "err", notifyErr) + } +}()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@notifier/notifier.go` at line 327, The current goroutine call drops Notify's error ("go func() { _ = Notify(err, list...) }()"); change it to capture the return value and log failures inside the goroutine so async notify failures are visible but control flow is unchanged—e.g. in the anonymous goroutine call to Notify(err, list...) check if err := Notify(...); err != nil { /* use existing logger (e.g., notifier.logger or package log) to log error with context like "async Notify failed" and the original err and err/list info */ }..golangci.yml (1)
11-17: Narrow test exclusions for high-signal linters.Line [11]-[17] disables
errcheckandstaticcheckfor every test file. That tends to hide real test defects; prefer targeted suppressions instead of global_test.gobypasses.Suggested config adjustment
rules: - linters: - funlen - goconst - - errcheck - - ineffassign - - staticcheck + - ineffassign path: (.+)_test\.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.golangci.yml around lines 11 - 17, The YAML currently disables high-signal linters (errcheck, staticcheck) for all test files by listing them under linters for path pattern "(+)_test\.go"; instead, restrict the exclusion to specific test files or folders that truly need it and remove errcheck and staticcheck from the broad linters list. Locate the block that names linters (funlen, goconst, errcheck, ineffassign, staticcheck) and the path: (.+)_test\.go pattern and change the path to targeted patterns (e.g., specific filenames or a tests/compat/ directory) or remove errcheck/staticcheck from that exclusion so those linters run on most _test.go files; keep only minimal, justified exclusions for known false positives.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Line 1: The Makefile's .PHONY declaration is missing the required 'all' and
'clean' targets causing checkmake warnings; update the .PHONY line (or add a
.PHONY entry) to include all and clean alongside existing targets so that .PHONY
lists ".PHONY: all clean build test doc lint bench" and ensure corresponding
'all' and 'clean' targets exist or are no-ops if not needed.
In `@notifier/notifier.go`:
- Around line 80-85: InitSentry currently returns on raven.SetDSN error without
clearing prior state, so sentryInited can remain true after a failed re-init;
update InitSentry to explicitly set sentryInited = false before returning on any
error from raven.SetDSN (and any other failure paths) so stale init state is
cleared; locate the InitSentry function and the sentryInited variable and ensure
every early return on setup failure resets sentryInited to false.
---
Outside diff comments:
In @.github/workflows/go.yml:
- Around line 6-16: The workflow relies on default GITHUB_TOKEN permissions; add
an explicit least-privilege permissions block at the top-level of the workflow
(above jobs) to restrict token scopes for the CI run. Add a permissions map
(e.g., permissions: contents: read, actions: read, packages: read) to limit
access for the GITHUB_TOKEN used by the build job and other steps; ensure the
block is applied at the workflow root (not inside job 'build') so all steps use
the reduced scopes.
---
Nitpick comments:
In @.golangci.yml:
- Around line 11-17: The YAML currently disables high-signal linters (errcheck,
staticcheck) for all test files by listing them under linters for path pattern
"(+)_test\.go"; instead, restrict the exclusion to specific test files or
folders that truly need it and remove errcheck and staticcheck from the broad
linters list. Locate the block that names linters (funlen, goconst, errcheck,
ineffassign, staticcheck) and the path: (.+)_test\.go pattern and change the
path to targeted patterns (e.g., specific filenames or a tests/compat/
directory) or remove errcheck/staticcheck from that exclusion so those linters
run on most _test.go files; keep only minimal, justified exclusions for known
false positives.
In `@notifier/notifier.go`:
- Line 327: The current goroutine call drops Notify's error ("go func() { _ =
Notify(err, list...) }()"); change it to capture the return value and log
failures inside the goroutine so async notify failures are visible but control
flow is unchanged—e.g. in the anonymous goroutine call to Notify(err, list...)
check if err := Notify(...); err != nil { /* use existing logger (e.g.,
notifier.logger or package log) to log error with context like "async Notify
failed" and the original err and err/list info */ }.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4605f2a7-d91a-4824-b2a2-3b2c930c10f7
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
.github/workflows/go.yml.golangci.ymlMakefileerrors.gogo.modnotifier/notifier.go
- Add permissions: contents: read to CI workflow - Fix go test flag order (-race before packages) - Fix bench target to skip regular tests (-run=^$) - Reset sentryInited on InitSentry failure
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
fmt.Errorfvet issue (useerrors.Newfor non-format strings)raven.SetDSNerror, merge var declaration, use switch-raceflag to test targetTest plan
make buildpassesmake testpasses (with -race)make lintpasses with 0 issuesSummary by CodeRabbit
New Features
Improvements