Conversation
- Bump Go version from 1.24.0 to 1.25.0. - Add new dependencies: `github.com/valyala/fasthttp` and `github.com/andybalholm/brotli`. - Update existing dependencies to their latest versions. - Refactor HTTP server implementation to support both `net/http` and `fasthttp`. - Introduce access logging and health check endpoints for improved observability. - Remove deprecated `init.go` file and related Gin setup. - Add tests for Gin framework integration and server behaviors.
- Updated README to include Gin as an option for serving HTTP alongside net/http and fasthttp. - Refactored HTTP server constructors to accept port and probe checks directly, improving usability. - Introduced NewGin and NewFastHTTP convenience constructors for easier server setup. - Updated tests to cover new Gin server behaviors and ensure compatibility with existing routes. - Renamed AlwaysOk to CheckOK for clarity in health check implementations.
There was a problem hiding this comment.
Pull request overview
This PR refactors the httpserver package from a Gin-only API into a shared server lifecycle supporting net/http, Gin, and native fasthttp, and adds migration documentation plus expanded integration/unit tests.
Changes:
- Introduces
httpserver.Serverlifecycle (ListenAndServe/Shutdown) and probe checks viaCheck. - Adds new constructors:
New/NewWithConfig(net/http),NewGin/NewGinWithConfig(Gin), andNewFastHTTP/NewFastHTTPWithConfig(fasthttp). - Adds an upgrade guide (
MIGRATION.md) and replaces/expands tests to cover TLS, probes, recovery, and lifecycle behavior.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates usage docs/examples for net/http, Gin, and fasthttp constructors. |
| MIGRATION.md | Adds upgrade guide mapping old Gin-only API to new constructors/types. |
| httpserver/tls_test.go | Removes old TLS test (replaced by consolidated tests). |
| httpserver/server.go | Introduces shared Server interface, Check probes, and shared TLS/port helpers; updates Listen to operate on the shared lifecycle. |
| httpserver/server_test.go | Adds consolidated tests for TLS, probes, recovery, and lifecycle for net/http and fasthttp. |
| httpserver/init.go | Removes Gin-specific global init/logger (replaced by new Gin/http logging setup). |
| httpserver/http.go | Adds net/http server implementation (wrapping handler with probes, recovery, access logging, TLS). |
| httpserver/gin.go | Adds Gin constructors + GinConfig to preserve minimal migration path. |
| httpserver/gin_test.go | Adds Gin constructor behavior tests and verifies Gin engine still works as http.Handler. |
| httpserver/fasthttp.go | Adds native fasthttp server implementation with probes, recovery, access logging, and TLS. |
| httpserver/accesslog.go | Adds shared access-log helpers and log threshold synchronization. |
| go.mod | Bumps Go version and adds fasthttp dependency (plus indirect dependency updates). |
| go.sum | Updates dependency checksums to match new/updated dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ssLogFor - Updated NewFastHTTP, NewGin, and New constructors to utilize slices.Clone for the DisableAccessLogFor field, ensuring a proper copy of the slice is made. - Enhanced TestTLS to use a local ephemeral listener for improved test isolation and reliability.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
httpserver/http.go:14
- accessLogHTTP intends to log a client IP, but the file doesn't import net, so it can't safely split request.RemoteAddr into host/port. Add the net import so RemoteAddr can be normalized to an IP-only value before logging.
import (
"context"
"errors"
"net/http"
"slices"
"time"
golog "log"
"github.com/rs/zerolog/log"
"github.com/trivago/go-bootstrap/logging"
)
httpserver/http.go:169
- The access log's "clientip" field is populated from request.RemoteAddr, which includes the client port (and may be bracketed for IPv6). This makes logs inconsistent with Gin/fasthttp (which log IP only) and with the field name. Split RemoteAddr with net.SplitHostPort and fall back to RemoteAddr if parsing fails.
writeAccessLog(
ignorePaths,
request.URL.Path,
recorder.status,
request.Method,
request.RemoteAddr,
time.Since(started),
"",
)
httpserver/server.go:87
- Listen closes signalChan when the server goroutine exits (after ListenAndServe returns), so a closed channel here means the server has already stopped—not that it failed to start. The comment is misleading and should be updated to reflect the actual control flow.
// React on external OS signals to trigger a shutdown.
// If the channel was closed, the server did not start.
if sig, isOpen := <-signalChan; isOpen {
httpserver/server.go:78
- Listen logs "Failed to start HTTP server" for any ListenAndServe error, even if the server ran and later exited unexpectedly. This message is misleading; prefer wording that reflects a runtime exit rather than only startup failure.
if err := srv.ListenAndServe(); err != nil {
log.Error().Err(err).Msg("Failed to start HTTP server")
}
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.
Extend the HTTP server package with net/http and native fasthttp support while preserving a minimal migration path for existing Gin applications. Add shared lifecycle, probes, access logging, recovery, TLS handling, integration tests, and an upgrade guide for the breaking API changes.
conventional commits
feat!(httpserver): add net/http and fasthttp server implementations
feat(httpserver): add Gin constructors for minimal migration
chore(httpserver): add server and framework integration tests
chore(docs): document server usage and migration paths
chore(deps): update Go dependencies and add fasthttp