refactor: use Go 1.22's ServeMux path patterns instead gorilla/mux#684
Draft
benhoyt wants to merge 3 commits into
Draft
refactor: use Go 1.22's ServeMux path patterns instead gorilla/mux#684benhoyt wants to merge 3 commits into
benhoyt wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull Request Overview
Refactors Pebble's HTTP routing from gorilla/mux to Go 1.22's native ServeMux path patterns to improve performance and reduce dependencies.
- Replaced gorilla/mux with http.ServeMux for HTTP routing
- Updated path parameter extraction from
muxVars(req)["param"]toreq.PathValue("param") - Simplified path patterns to match ServeMux syntax (e.g.,
{task-id}to{taskID})
Reviewed Changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internals/daemon/daemon.go | Replace mux.Router with http.ServeMux and simplify route registration |
| internals/daemon/api.go | Remove mux dependency and update path patterns to use camelCase |
| internals/daemon/api_changes.go | Update parameter extraction to use req.PathValue() |
| internals/daemon/api_notices.go | Update parameter extraction to use req.PathValue() |
| internals/daemon/api_tasks.go | Update parameter extraction to use req.PathValue() |
| internals/daemon/daemon_test.go | Refactor tests to use HTTP requests instead of mux internals |
| internals/daemon/export_test.go | Remove mux-related test utilities |
| internals/daemon/api_test.go | Remove mux test setup and helper functions |
| internals/daemon/api_changes_test.go | Update tests to use req.SetPathValue() |
| internals/daemon/api_notices_test.go | Update tests to use req.SetPathValue() |
| go.mod | Remove gorilla/mux dependency |
| docs/specs/openapi.yaml | Update API documentation to reflect new path patterns |
| docs/explanation/api-and-clients.md | Update documentation examples |
| HACKING.md | Remove mux import from example code |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Path string | ||
| PathPrefix string | ||
| // | ||
| Path string |
There was a problem hiding this comment.
The PathPrefix field has been removed from the Command struct, but there's no clear indication of how prefix-based routing is now handled. If any commands previously used PathPrefix, this could be a breaking change.
Suggested change
| Path string | |
| Path string | |
| PathPrefix string |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DRAFT: not yet ready for review. And I still need to figure out how this applies to snapd and run it past them.
See also: draft of using Copilot to do this in the snapd repo
Oddly, it doesn't decrease the binary size. In fact, building with the flags we use to build the snap, it's very slightly bigger (exactly 4096 = 4KB). Not sure why, maybe something's bumped it into using another 4KB "page".
In terms of performance, routing is 30% faster, and does fewer memory allocations. Not that that is a big driver here, both are at the millisecond level. But here are the numbers of this benchmark:
Fixes #676.