Conversation
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
📝 WalkthroughWalkthroughThis PR adds lazy, concurrency-safe initialization for the gzip middleware, explicitly disables zstd while enabling gzip, updates the Go toolchain to 1.26, adds package doc files for plugin and tests, and updates revive linter settings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
Updates the RoadRunner gzip plugin to ensure only gzip compression is used, addressing roadrunner-server/roadrunner#2293, while also adjusting Go/tooling and lint configuration.
Changes:
- Replace
gzhttp.GzipHandlerusage with agzhttp.NewWrapper(...)configured to disable Zstd. - Bump workspace/module Go version declarations and remove the root
toolchainpin. - Enable Revive default rules and add missing package doc stubs to satisfy linting expectations.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
plugin.go |
Wrap middleware with a lazily-initialized gzhttp wrapper configured to disable Zstd. |
go.mod |
Updates go directive to 1.26 and removes toolchain directive. |
go.work |
Updates workspace go directive to 1.26. |
.golangci.yml |
Enables Revive default rules (with var-naming disabled). |
doc.go |
Adds package documentation comment for gzip package. |
tests/doc.go |
Adds package documentation comment for tests package. |
tests/mock/doc.go |
Adds package documentation comment for mock logger package. |
Comments suppressed due to low confidence (1)
plugin.go:28
- The doc comment refers to
GzipHandler, but the function is namedgzipHandler(unexported). Please update the comment to match the identifier name (and consider rephrasing “allows to easily” for correct grammar).
// GzipHandler allows to easily wrap an http handler with default settings.
func gzipHandler(h http.Handler) http.HandlerFunc {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,4 +1,4 @@ | |||
| go 1.25 | |||
| go 1.26 | |||
There was a problem hiding this comment.
Bumping the workspace go version to 1.26 changes the minimum Go toolchain needed to work in this repo (including the ./tests module used in the workspace). Please confirm this is intended and kept consistent with the module go.mod / CI toolchains.
| go 1.26 | |
| go 1.22 |
| revive: | ||
| enable-default-rules: true | ||
| rules: | ||
| - name: var-naming | ||
| disabled: true |
There was a problem hiding this comment.
Enabling Revive’s default rules can introduce new lint failures and effectively changes the repo’s lint policy. Since the PR description is about gzip-only compression, consider calling this out explicitly (or splitting lint-policy changes into a separate PR) to keep scope clear.
| var onceDefault sync.Once //nolint:gochecknoglobals | ||
| var defaultWrapper func(http.Handler) http.HandlerFunc //nolint:gochecknoglobals | ||
|
|
||
| // GzipHandler allows to easily wrap an http handler with default settings. | ||
| func gzipHandler(h http.Handler) http.HandlerFunc { | ||
| onceDefault.Do(func() { | ||
| var err error | ||
| defaultWrapper, err = gzhttp.NewWrapper(gzhttp.PreferZstd(false), gzhttp.EnableZstd(false), gzhttp.EnableGzip(true)) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| }) |
There was a problem hiding this comment.
gzhttp.NewWrapper errors are currently handled via panic, which can crash the whole process at runtime. Consider constructing the wrapper during (*Plugin).Init() (storing it on the Plugin struct) and returning the error from Init, which also avoids the need for global state and //nolint:gochecknoglobals.
![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
🧹 Nitpick comments (3)
plugin.go (3)
24-25: Group related package-level variables into a singlevarblock.Two consecutive top-level
vardeclarations for tightly coupled variables is unconventional; grouping them is the idiomatic Go style and allows a single//nolintdirective.✏️ Proposed grouping
-var onceDefault sync.Once //nolint:gochecknoglobals -var defaultWrapper func(http.Handler) http.HandlerFunc //nolint:gochecknoglobals +var ( //nolint:gochecknoglobals + onceDefault sync.Once + defaultWrapper func(http.Handler) http.HandlerFunc +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin.go` around lines 24 - 25, Group the two package-level variables into a single var block: replace the separate declarations of onceDefault and defaultWrapper with one var (...) block containing both onceDefault sync.Once and defaultWrapper func(http.Handler) http.HandlerFunc, and move the single //nolint:gochecknoglobals comment to apply to that var block so the linter exemption covers both symbols.
31-31:PreferZstd(false)andEnableGzip(true)are redundant.
EnableGzipenables or disables gzip compression, and it is enabled by default. ThePreferZstdoption controls which codec is preferred when the client accepts both with equal quality values. SinceEnableZstd(false)already disables zstd entirely, the tie-breakingPreferZstd(false)has no effect. Dropping both redundant options makes the intent clearer: onlyEnableZstd(false)is needed to enforce gzip-only behavior.✏️ Proposed simplification
- defaultWrapper, err = gzhttp.NewWrapper(gzhttp.PreferZstd(false), gzhttp.EnableZstd(false), gzhttp.EnableGzip(true)) + defaultWrapper, err = gzhttp.NewWrapper(gzhttp.EnableZstd(false))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin.go` at line 31, Remove the redundant gzip-related options when constructing the wrapper: in the call to gzhttp.NewWrapper that sets defaultWrapper, drop gzhttp.PreferZstd(false) and gzhttp.EnableGzip(true) and keep only gzhttp.EnableZstd(false) so the intent (disable zstd, use gzip default) is clear; update the invocation of gzhttp.NewWrapper accordingly around the defaultWrapper variable.
27-27: Doc comment mentionsGzipHandlerbut function isgzipHandler.Go's doc-comment convention requires the first word to match the declared identifier. The current comment will not render correctly with
go docorgodoc.✏️ Proposed fix
-// GzipHandler allows to easily wrap an http handler with default settings. +// gzipHandler allows to easily wrap an http handler with default settings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugin.go` at line 27, The doc comment's first word must match the declared identifier: update the comment to start with "gzipHandler" to match the unexported function name gzipHandler, or if the function was meant to be exported, rename the function to GzipHandler and keep the comment as-is; ensure the chosen identifier (gzipHandler or GzipHandler) is used consistently in the comment so it follows Go's godoc convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugin.go`:
- Around line 29-35: The sync.Once initializer currently panics inside
onceDefault.Do (calling gzhttp.NewWrapper) which can leave defaultWrapper nil if
the panic is recovered; change this by moving the wrapper initialization into
the package Init() function (call gzhttp.NewWrapper there, handle the error with
a fatal log or return an error) and then simplify Middleware to call
defaultWrapper(h) directly; remove gzipHandler, onceDefault, and the sync import
after moving initialization so there is no panic inside Do and error handling is
explicit.
---
Nitpick comments:
In `@plugin.go`:
- Around line 24-25: Group the two package-level variables into a single var
block: replace the separate declarations of onceDefault and defaultWrapper with
one var (...) block containing both onceDefault sync.Once and defaultWrapper
func(http.Handler) http.HandlerFunc, and move the single
//nolint:gochecknoglobals comment to apply to that var block so the linter
exemption covers both symbols.
- Line 31: Remove the redundant gzip-related options when constructing the
wrapper: in the call to gzhttp.NewWrapper that sets defaultWrapper, drop
gzhttp.PreferZstd(false) and gzhttp.EnableGzip(true) and keep only
gzhttp.EnableZstd(false) so the intent (disable zstd, use gzip default) is
clear; update the invocation of gzhttp.NewWrapper accordingly around the
defaultWrapper variable.
- Line 27: The doc comment's first word must match the declared identifier:
update the comment to start with "gzipHandler" to match the unexported function
name gzipHandler, or if the function was meant to be exported, rename the
function to GzipHandler and keep the comment as-is; ensure the chosen identifier
(gzipHandler or GzipHandler) is used consistently in the comment so it follows
Go's godoc convention.
Signed-off-by: Valery Piashchynski <piashchynski.valery@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #212 +/- ##
===========================================
+ Coverage 0 56.52% +56.52%
===========================================
Files 0 1 +1
Lines 0 23 +23
===========================================
+ Hits 0 13 +13
- Misses 0 8 +8
- Partials 0 2 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/linux.yml (1)
82-88: Hardcoded module path in theawkfilter will silently drop all coverage on a major-version bump.The pattern
/^github\.com\/roadrunner-server\/gzip\/v5\//hardcodes the module major version (v5). If the module is ever bumped tov6, every coverage line will fail to match, thesubwill never fire, andsummary.txtwill contain only themode: atomicheader — producing a zero-coverage report with no error surfacing from eitherawkorcodecov-action(sincefail_ci_if_error: false).♻️ Suggested fix: derive the prefix dynamically
- tail -q -n +2 coverage/*.out >> summary.txt - awk ' - NR == 1 { print; next } - /^github\.com\/roadrunner-server\/gzip\/v5\// { - sub(/^github\.com\/roadrunner-server\/gzip\/v5\//, "", $0) - print - } - ' summary.txt > summary.filtered.txt - mv summary.filtered.txt summary.txt + tail -q -n +2 coverage/*.out >> summary.txt + MODULE=$(go list -m) + awk -v mod="$MODULE/" ' + NR == 1 { print; next } + index($0, mod) == 1 { + sub(mod, "", $0) + print + } + ' summary.txt > summary.filtered.txt + mv summary.filtered.txt summary.txt
go list -mreads the module path directly fromgo.mod, so the filter stays correct across any future major version change without a manual edit. Thecodecovjob already has the repo checked out (Line 72–73), sogois available viaactions/setup-goin the prior job — thoughgomust also be available in thecodecovjob. If preferred, the prefix can be extracted from the first coverage file's path instead:+ MODULE_PREFIX=$(head -2 summary.txt | tail -1 | grep -oP '^[^:]+' | sed 's|/[^/]*$|/|')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/linux.yml around lines 82 - 88, The awk filter is hardcoded to the module major version via the pattern /^github\.com\/roadrunner-server\/gzip\/v5\// which will stop matching on a major-version bump; update the pipeline so the prefix is derived dynamically (e.g., call `go list -m` to get the module path or extract the prefix from the first coverage file path) and then use that computed prefix in the awk substitution that transforms summary.txt into summary.filtered.txt; ensure the computed prefix replaces the literal `github\.com\/roadrunner-server\/gzip\/v5\//` in the awk script so the sub(...) runs correctly across major-version changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/linux.yml:
- Around line 82-88: The awk filter is hardcoded to the module major version via
the pattern /^github\.com\/roadrunner-server\/gzip\/v5\// which will stop
matching on a major-version bump; update the pipeline so the prefix is derived
dynamically (e.g., call `go list -m` to get the module path or extract the
prefix from the first coverage file path) and then use that computed prefix in
the awk substitution that transforms summary.txt into summary.filtered.txt;
ensure the computed prefix replaces the literal
`github\.com\/roadrunner-server\/gzip\/v5\//` in the awk script so the sub(...)
runs correctly across major-version changes.
Reason for This PR
closes: roadrunner-server/roadrunner#2293
Description of Changes
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
Chores
Documentation
CI