Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR upgrades Echo v4→v5, adds an afero filesystem to server Start, introduces a cancellable server context, changes handler signatures to use *echo.Context, switches logging to structured slog, updates tests for in-memory TLS certs, and adds Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI / serve.go"
participant Server as "internal/api/server.go\nServer"
participant FS as "afero.Fs (IOFS)"
participant Echo as "Echo v5 Engine"
CLI->>Server: Start(fs, address, useSSL, certFile, keyFile)
Server->>FS: open certFile / keyFile via IOFS
Server->>Echo: configure Start/StartTLS with StartConfig + ctx
Echo-->>Server: server started / running
CLI->>Server: Stop()
Server->>Server: invoke cancel()
Echo-->>Server: graceful shutdown
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/api/handlers/root_test.go (1)
152-164: Assertion message is out of sync with the expected count.The message says “Expected 2 routes” while
lenExpectedRoutesis 3. UsingLenfkeeps the message accurate.🛠️ Suggested fix
- assert.Len(t, e.Router().Routes(), lenExpectedRoutes, "Expected 2 routes to be registered") + assert.Lenf(t, e.Router().Routes(), lenExpectedRoutes, "Expected %d routes to be registered", lenExpectedRoutes)internal/api/handlers/root.go (1)
40-56: Swagger route is disabled while Root still redirects to a missing endpoint.The commented-out
/swagger/*registration means tests expecting this route will fail. TheRoothandler redirects to/swagger/index.html, but that endpoint is never registered. Additionally,echo-swagger(swaggo/echo-swagger) targets Echo v4 and lacks official v5 support—your dependencies include both v4 and v5, suggesting a migration in progress. You must either:
- Re-enable the swagger handler and resolve the v5 compatibility (fork or wait for community port), or
- Update
Rootand tests to remove the swagger redirectAlso, if the comment line remains, add a space after
//to satisfy lint rules:Lint fix for the commented line
- //g.GET("/swagger/*", echoSwagger.WrapHandler) + // g.GET("/swagger/*", echoSwagger.WrapHandler)
🤖 Fix all issues with AI agents
In `@cmd/upswake/root_test.go`:
- Around line 208-210: The test uses the literal "http(s) server started" in the
wantOutput slice which will never match actual Echo logs ("http server started"
or "https server started"); update the assertions that reference wantOutput (the
test's wantOutput slice used in the test function in root_test.go) to perform a
scheme‑agnostic match—either replace the literal with a regex like `^https?
server started` and use assert.Regexp (or similar) to check the log line, or
split the expectation into two explicit strings and assert the actual output
equals one of them depending on useSSL; ensure you update both occurrences that
currently contain "http(s) server started".
In `@go.mod`:
- Around line 9-10: Remove the unused Echo v4 module entry from go.mod by
deleting the line declaring github.com/labstack/echo/v4 v4.9.0, then run go mod
tidy to update module graph and vendor files; verify that all imports reference
labstack/echo/v5 (e.g., in server.go and handler packages) and run the
test/build to ensure no references remain to Echo v4.
In `@internal/api/handlers/servers.go`:
- Around line 80-84: Echo v5 removed Context.Logger(), so replace uses of
c.Logger() in handlers (e.g., WakeServer and other methods on ServerHandler)
with the supported pattern: use c.Echo().Logger (or retrieve a logger stored in
context via c.Get) and update log calls like c.Logger().Error(...) to
c.Echo().Logger.Error(...) (or the context logger retrieval) and adjust any
structured logging helpers to use slog.Any/fields as before; ensure all
occurrences across the handlers are changed consistently.
In `@internal/api/server_test.go`:
- Around line 274-286: Remove the noisy certificate print and stale TODO: delete
the fmt.Println(string(publicRSAKey)) call and remove any associated TODO
comment referencing use of an in-memory FS; the cert setup using certFs,
generateEncodedRSAKeys, generateEncodedECCKeys and the afero.WriteFile calls
should remain untouched so tests continue writing rsa.key/rsa.cert and
ecc.key/ecc.cert to the memory FS without printing their contents (also apply
same removal at the other occurrence around lines 405-406).
- Around line 200-266: The RSA PEM block in generateEncodedRSAKeys uses
x509.MarshalPKCS1PrivateKey (PKCS#1) but labels the PEM as "PRIVATE KEY", which
implies PKCS#8; update generateEncodedRSAKeys to either change the PEM block
Type to "RSA PRIVATE KEY" to match PKCS#1 (recommended) or convert the key to
PKCS#8 via x509.MarshalPKCS8 and keep "PRIVATE KEY"; ensure the change is
applied where x509.MarshalPKCS1PrivateKey and pem.Encode are used so TLS loaders
accept the key.
In `@internal/api/server.go`:
- Around line 84-103: The GracefulTimeout in Server.Start is set to the bare
integer 5 (nanoseconds) causing improper shutdown; update the echo.StartConfig
initialization in Start (the start variable) to use an explicit time.Duration
such as 5 * time.Second for the GracefulTimeout field and add the time package
import; ensure you modify the Start function and import list so GracefulTimeout:
5 * time.Second is used instead of 5.
🧹 Nitpick comments (2)
internal/api/handlers/upswake.go (1)
74-135: Echo logger may stringifyslog.Attr, losing structure.If you want structured logs, consider logging via
slog.Loggerdirectly or ensure Echo’s logger is wired to a slog handler so attributes remain structured; otherwise they’ll likely be rendered as plain strings.internal/api/handlers/servers.go (1)
150-154: Inconsistent error logging - consider adding context.This error log lacks structured context compared to other error logs in this file. Since
broadcastis available in scope, including it would aid debugging.🔧 Suggested improvement
if broadcast == nil { - c.Logger().Error("invalid broadcast address") + c.Logger().Error("invalid broadcast address encountered", slog.Int("index", /* loop index */), slog.Any("broadcast", broadcast)) return c.JSON(http.StatusInternalServerError, Response{Message: ErrorBroadcastAddress.Error()}) }Alternatively, if you want to include the loop index, change to an indexed loop:
-for _, broadcast := range broadcasts { +for i, broadcast := range broadcasts {
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@internal/api/handlers/root.go`:
- Around line 40-44: The Register function currently comments out the Swagger
route causing RootHandler.Root's redirect to /swagger/index.html to 404; either
re-enable the Swagger route in RootHandler.Register by uncommenting or restoring
the g.GET("/swagger/*", echoSwagger.WrapHandler) line so the /swagger/* handler
is registered, or update RootHandler.Root to redirect/serve a path that exists;
locate RootHandler.Register and RootHandler.Root in
internal/api/handlers/root.go and make the change so the route referenced by
RootHandler.Root is actually registered (tests expect the /swagger/* route to be
present).
In `@internal/api/server_test.go`:
- Around line 32-34: The pingHandler currently uses the wrong Echo v5 signature
(func pingHandler(c *echo.Context) error); change it to the correct interface
type by updating the signature to use echo.Context (func pingHandler(c
echo.Context) error) and update any local references inside pingHandler that
assume a pointer receiver to use the interface methods instead so it compiles
against Echo v5.
♻️ Duplicate comments (3)
internal/api/server_test.go (3)
236-239: RSA PEM label mismatches the encoded key format.The key is encoded using
x509.MarshalPKCS1PrivateKey(PKCS#1 format), but the PEM block type is"PRIVATE KEY"which indicates PKCS#8 format. This mismatch may cause TLS loaders to fail. Use"RSA PRIVATE KEY"for PKCS#1.Proposed fix
- err = pem.Encode(&pemKey, &pem.Block{Type: "PRIVATE KEY", Bytes: b}) + err = pem.Encode(&pemKey, &pem.Block{Type: "RSA PRIVATE KEY", Bytes: b})
286-286: Remove debug print statement.Printing certificate material clutters test logs and serves no purpose in automated tests.
Proposed fix
- fmt.Println(string(publicRSAKey))
405-406: Remove stale TODO comment.The TODO states "We should use a memory fs here" but the code already uses
certFs(an in-memory filesystem). This comment is now misleading.Proposed fix
- // TODO: We should use a memory fs here with certificatse loaded, not depending on actual certs on disk err := srv.Start(certFs, tt.args.address, tt.args.useSSL, tt.args.certFile, tt.args.keyFile)
TheDarthMole
force-pushed
the
chore/update-echo-v5
branch
from
346c660 to
e362a73
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@internal/api/server.go`:
- Around line 42-45: The validator is being created with the parent ctx instead
of the server's cancellable context newCtx, so change the NewServer function to
pass newCtx into NewCustomValidator (so app.Validator =
NewCustomValidator(newCtx)) ensuring the CustomValidator used by app.Validator
and its Validate method (which calls cv.validator.StructCtx(cv.ctx, i)) is tied
to the server's cancellation lifecycle.
🧹 Nitpick comments (6)
internal/api/handlers/root.go (1)
76-76: Consider using structured logging instead of string concatenation.Concatenating
err.Error()into the message string loses the ability to filter/parse the error field independently in structured log aggregators. Echo v5'sLoggerinterface is minimal, so this may be acceptable, but if you have access toslogorzaphere, structured fields would be preferable.- c.Logger().Error("Error getting broadcast addresses: " + err.Error()) + // If a richer logger is available via dependency injection: + // logger.Error("Error getting broadcast addresses", "error", err)Also applies to: 83-83
internal/api/server_test.go (1)
390-410: Parallel tests sharing randomised ports carry a small collision risk.Each test case generates a random port via
rand.IntN(65535-49152)+49152. Since tests run in parallel (line 392), two tests could theoretically pick the same port. The probability is low given the range (~16k ports), but for robustness you could use:0to let the OS assign a free port, or retry onEADDRINUSE.This is a minor observation — unlikely to cause flakes in practice.
internal/api/server.go (2)
94-96:HTTPSRedirectmiddleware is registered on everyStartcall with SSL.If
Startwere called multiple times (unlikely but possible), thePre(middleware.HTTPSRedirect())middleware would be registered repeatedly. SinceStartis effectively called once, this is fine in practice, but registering middleware inNewServer(conditionally) or guarding against double-registration would be more defensive.
102-105:Stop()always returnsnil— consider whether callers expect meaningful errors.Currently
Stoponly cancels the context and returnsnil. The caller inserve.go(line 121-123) checks the error fromStop()and logs a warning. SinceStopcan never return an error, that check is dead code. IfStopis never expected to fail, consider changing the signature to not return an error, or document the contract.internal/api/handlers/servers.go (2)
12-12: Advisory: Echo v5 is in a pre-stability period until 2026-03-31.Echo recommends waiting until after 2026-03-31 before upgrading to v5 in production, as breaking API changes may still occur during this period, even if it violates semantic versioning. Ensure this timeline aligns with your release plans, or pin to a specific v5 tag to avoid surprise breakage.
112-113: Consider loggingportandbroadcastfor consistency withBroadcastWakeServer.The
BroadcastWakeServerhandler (lines 174–177) logsmac,port, andbroadcastfields on success, whereasWakeServeronly logsmac(line 112). SinceWakeServerRequestcontains all three fields, addingportandbroadcastto the success log would improve debuggability and maintain consistent observability across both handlers.Suggested improvement
- c.Logger().Info("wake on lan packet sent", slog.String("mac", sanitizeString(wsRequest.Mac))) + c.Logger().Info("wake on lan packet sent", + slog.String("mac", sanitizeString(wsRequest.Mac)), + slog.Int("port", wsRequest.Port), + slog.String("broadcast", wsRequest.Broadcast))
TheDarthMole
force-pushed
the
chore/update-echo-v5
branch
from
2e0bc1f to
921344a
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
1 participant
Dependent on swaggo/echo-swagger#140
Summary by CodeRabbit
New Features
Refactor
Changes
Tests
Chores