chore: Refactor main startup for idiomatic Go #150
Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughReplaces init-based boot with explicit main() calling run() that returns errors; centralizes environment, Sentry, DB and app bootstrap; kernel.Ignite now returns (*env.Environment, error); adds endpoint.RunServer and NewServerHandler for server lifecycle and graceful shutdown; refactors CLIs and seeders to dependency-inject and propagate errors. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant M as main()
participant R as run()
participant K as kernel.Ignite()
participant A as kernel.NewApp()
participant H as endpoint.NewServerHandler()
participant S as endpoint.RunServer()/http.Server
participant OS as OS Signals
M->>R: call run()
R->>K: Ignite(envPath, validator)
alt Ignite returns error
K-->>R: error
R-->>M: return error
M->>M: log/capture Sentry & exit(1)
else Ignite ok
K-->>R: env
R->>A: NewApp(env,...)
A-->>R: app
R->>H: NewServerHandler(cfg)
H-->>R: handler
R->>S: RunServer(addr, server) (goroutine)
par Serve & signal watcher
S-->>R: ListenAndServe() runs
OS->>R: SIGINT/SIGTERM
R->>S: Shutdown(ctx)
alt shutdown exceeds deadline
R->>S: Close() (force)
end
end
R-->>M: return nil
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
Summary of ChangesHello @gocanto, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the application's startup process to align with more idiomatic Go practices. The changes aim to enhance the robustness, clarity, and maintainability of the main entry point by improving error handling, removing global state, and refining logging, ultimately leading to a more stable and predictable application lifecycle. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the application's startup logic by adopting more idiomatic Go practices, such as removing the global application singleton and using a run function that returns errors. The changes make the startup process cleaner and more robust. My review includes a couple of suggestions to further enhance this robustness: one regarding fail-fast behavior on database connection failure, and another to align an upstream function call with the new error-handling pattern.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.go (1)
3-16: Add imports for URL parsing used in ListenAndServe address derivation.
To safely strip scheme from the host URL, you'll need these:import ( "errors" "fmt" "log/slog" "net/http" + "net/url" "os" + "strings" "time" "github.com/getsentry/sentry-go" _ "github.com/lib/pq" "github.com/oullin/metal/kernel" "github.com/oullin/pkg/portal" "github.com/rs/cors" )
🧹 Nitpick comments (5)
main.go (5)
49-55: Consider http.Server with timeouts and signal-based graceful shutdown.
Better resilience and proper http.ErrServerClosed handling.Example sketch (outside current diff scope):
srv := &http.Server{ Addr: addr, Handler: serverHandler(app), ReadTimeout: 15 * time.Second, WriteTimeout: 15 * time.Second, IdleTimeout: 60 * time.Second, } go func() { if err := srv.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { slog.Error("listen and serve failed", "error", err) } }() // Listen for SIGINT/SIGTERM, then: // ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) // defer cancel() // return srv.Shutdown(ctx)
60-64: Nil-app handler choice: 404 vs 500 (optional).
Returning 404 hides internals but semantically this is a 500. Up to you; just flagging.
72-75: Variable naming and origin clarity (optional).
The name localhost may be misleading if the value isn’t localhost. Consider origin or hostURL.- env := app.GetEnv() - localhost := env.Network.GetHostURL() + env := app.GetEnv() + origin := env.Network.GetHostURL()Also consider allowing 127.0.0.1 variants in dev.
92-101: Dev CORS: include 127.0.0.1 origin (optional).
Many dev setups use 127.0.0.1. Add it to improve DX.- c := cors.New(cors.Options{ - AllowedOrigins: []string{localhost, "http://localhost:5173"}, + c := cors.New(cors.Options{ + AllowedOrigins: []string{origin, "http://localhost:5173", "http://127.0.0.1:5173"},
103-105: Avoid shadowing the sentry package name.
Local var name sentry shadows the imported package; rename for clarity.- if sentry := app.GetSentry(); sentry != nil && sentry.Handler != nil { - handler = sentry.Handler.Handle(handler) + if s := app.GetSentry(); s != nil && s.Handler != nil { + handler = s.Handler.Handle(handler) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
main.go(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
main.go (3)
pkg/portal/validator.go (1)
GetDefaultValidator(23-33)metal/kernel/ignite.go (1)
Ignite(9-15)metal/kernel/app.go (2)
NewApp(26-42)App(17-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (1.25.3)
- GitHub Check: test (1.25.3)
🔇 Additional comments (1)
main.go (1)
46-55: The review comment is incorrect; GetHostURL() returns host:port format, not a full URL with scheme.The script output confirms that
GetHostURL()(defined inmetal/env/network.go:18) returnse.HttpHost + ":" + e.HttpPort, which produces"localhost:8080"format. The test atmetal/env/env_test.go:102verifies this. This is the exact formathttp.ListenAndServe()expects—no scheme parsing is needed. The current code inmain.go:49is correct as-is.Likely an incorrect or invalid review comment.
gocanto
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
database/seeder/importer/cmd/main.go (1)
37-64: Defer RecoverWithSentry before constructing DB/logs to capture panicsIf NewDbConnection/NewLogs panic, current defer won’t run. Defer recovery immediately after creating the Sentry hub.
func run(filePath string, environment *env.Environment, sentryHub *portal.Sentry) error { if !environment.App.IsLocal() { @@ cli.ClearScreen() - dbConnection := kernel.NewDbConnection(environment) - logs := kernel.NewLogs(environment) + defer kernel.RecoverWithSentry(sentryHub) + + dbConnection := kernel.NewDbConnection(environment) + logs := kernel.NewLogs(environment) defer logs.Close() defer dbConnection.Close() - defer kernel.RecoverWithSentry(sentryHub)
🧹 Nitpick comments (5)
metal/kernel/kernel_test.go (1)
124-135: Make the missing-file path portable; avoid hard-coded “/nonexistent/.env”Use a temp dir and join a non-existent filename to be OS-agnostic.
Apply this diff:
- _, err := Ignite("/nonexistent/.env", portal.GetDefaultValidator()) + path := filepath.Join(t.TempDir(), "missing.env") + _, err := Ignite(path, portal.GetDefaultValidator())Add the missing import:
import "path/filepath"database/seeder/importer/cmd/main.go (1)
19-27: Also capture and flush Sentry on fatal errors in main (consistency)Mirror other entrypoints: capture run() errors and flush before os.Exit.
- if err := run(defaultSQLDumpPath, environment, sentryHub); err != nil { - cli.Errorln(err.Error()) - os.Exit(1) - } + if err := run(defaultSQLDumpPath, environment, sentryHub); err != nil { + sentry.CurrentHub().CaptureException(err) + cli.Errorln(err.Error()) + sentry.Flush(2 * time.Second) + os.Exit(1) + }Also applies to: 31-35
metal/kernel/ignite.go (1)
11-17: Consider propagating validation errors instead of panicking in NewEnvIgnite still panics indirectly via NewEnv on invalid env. To fully embrace error-driven startup, refactor NewEnv to return (*env.Environment, error) and bubble errors up through Ignite.
metal/cli/main.go (1)
45-49: Nil check after NewDbConnection is ineffective (constructor panics on error)NewDbConnection panics on failure, so dbConn won’t be nil. Either:
- Refactor NewDbConnection to return (*database.Connection, error) and handle it here; or
- Drop the nil check to avoid dead code.
database/seeder/main.go (1)
42-53: Adopt error-returning factories instead of panics; nil checks won’t run on panicTo align with run()-returns-error style, have NewDbConnection/NewLogs return (value, error) and handle errors here. Current nil checks are ineffective if constructors panic.
Example pattern:
dbConnection, err := kernel.NewDbConnection(environment) // change signature to return (*database.Connection, error) if err != nil { return fmt.Errorf("connect db: %w", err) } defer dbConnection.Close() logs, err := kernel.NewLogs(environment) // change signature to return (llogs.Driver, error) if err != nil { return fmt.Errorf("open logs: %w", err) } defer logs.Close()Also applies to: 54-57
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
database/seeder/importer/cmd/main.go(1 hunks)database/seeder/main.go(3 hunks)main.go(3 hunks)metal/cli/main.go(6 hunks)metal/kernel/ignite.go(1 hunks)metal/kernel/kernel_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
main.go (3)
pkg/portal/validator.go (1)
GetDefaultValidator(23-33)metal/kernel/ignite.go (1)
Ignite(11-17)metal/kernel/app.go (2)
NewApp(26-42)App(17-24)
database/seeder/main.go (7)
pkg/cli/message.go (3)
Errorln(9-11)Error(5-7)Successln(17-19)pkg/cli/helpers.go (1)
ClearScreen(9-19)pkg/portal/validator.go (1)
GetDefaultValidator(23-33)metal/kernel/ignite.go (1)
Ignite(11-17)metal/kernel/factory.go (3)
NewSentry(16-44)NewDbConnection(46-54)NewLogs(56-64)metal/kernel/helpers.go (1)
RecoverWithSentry(41-49)database/seeder/seeds/factory.go (1)
NewSeeder(17-22)
metal/kernel/ignite.go (3)
pkg/portal/validator.go (1)
Validator(13-16)metal/env/env.go (1)
Environment(9-17)metal/kernel/factory.go (1)
NewEnv(66-163)
metal/kernel/kernel_test.go (2)
metal/kernel/ignite.go (1)
Ignite(11-17)pkg/portal/validator.go (1)
GetDefaultValidator(23-33)
metal/cli/main.go (11)
pkg/cli/message.go (3)
Errorln(9-11)Error(5-7)Successln(17-19)pkg/cli/helpers.go (1)
ClearScreen(9-19)pkg/portal/validator.go (1)
GetDefaultValidator(23-33)metal/kernel/ignite.go (1)
Ignite(11-17)metal/kernel/factory.go (2)
NewSentry(16-44)NewDbConnection(46-54)metal/kernel/helpers.go (1)
RecoverWithSentry(41-49)metal/cli/panel/menu.go (2)
NewMenu(27-36)Menu(19-23)database/connection.go (1)
Connection(12-17)metal/cli/accounts/factory.go (1)
NewHandler(19-34)metal/cli/posts/factory.go (1)
NewHandler(22-33)metal/cli/seo/generator.go (1)
Generator(31-39)
database/seeder/importer/cmd/main.go (4)
pkg/portal/validator.go (1)
GetDefaultValidator(23-33)metal/kernel/ignite.go (1)
Ignite(11-17)pkg/cli/message.go (2)
Errorln(9-11)Error(5-7)metal/kernel/factory.go (1)
NewSentry(16-44)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (1.25.3)
🔇 Additional comments (9)
metal/kernel/kernel_test.go (1)
114-117: Ignite error-return handling in tests looks goodCapturing both env and err and failing on non-nil error is correct.
metal/kernel/ignite.go (1)
11-17: Good: Ignite returns errors instead of panickingSignature and error wrapping are correct and align with the PR goals.
main.go (7)
3-19: LGTM! Imports are well-organized.All new imports (context, errors, os, os/signal, syscall) are standard library packages appropriately used for signal handling and graceful shutdown.
21-32: LGTM! Sentry error capture is correctly sequenced.The double-flush pattern (deferred at line 22, explicit after capture at line 26) ensures events are sent before exit. The timeout check with warning log is good practice for observability.
34-54: LGTM! Bootstrap sequence is correctly implemented.The initialization flow properly handles errors from
IgniteandNewApp, wraps them with context, and fails fast on database ping failure. This addresses the past review comment about database connectivity being essential.
56-82: LGTM! Server setup and signal handling follow Go best practices.The explicit timeouts on
http.Server(particularlyReadHeaderTimeout: 5s) mitigate slowloris attacks. The buffered error channel (size 1) prevents goroutine leaks. Signal handling for SIGINT and SIGTERM enables graceful shutdown on common termination signals.
83-121: LGTM! Graceful shutdown is robustly implemented.The shutdown flow correctly handles both server errors and OS signals. The 10-second grace period with fallback to force close (lines 104-108) prevents indefinite hangs. The switch statement (lines 100-111) explicitly handles expected vs unexpected shutdown errors, which aids in debugging and reliability.
123-139: LGTM! Defensive nil checks are good practice.The nil guards for
app(lines 124-126) andenv(lines 137-139) are defensive but enhance robustness. Whilerun()already validates these values, protecting against nil at handler construction prevents potential nil pointer panics in edge cases or during testing.
155-155: No issues found. TheX-API-Intended-Originheader is an intentional, required part of the API's authentication mechanism.The header is defined as a constant in
pkg/portal/consts.go, validated as required inpkg/middleware/token_middleware.go, used in signature validation, and tested. Adding it to the CORS allowlist is correct and consistent with the existing authentication flow.
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 (2)
docker/dockerfile-api (1)
120-123: Fix adduser invocation (username passed twice; options out of order).BusyBox adduser expects options before a single USER argument. Current form may fail or ignore flags.
Apply:
-RUN adduser -S ${APP_USER} \ - -G ${APP_GROUP} \ - -h ${APP_HOME} \ - ${APP_USER} +RUN adduser -S -G ${APP_GROUP} -h ${APP_HOME} ${APP_USER}docker-compose.yml (1)
91-97: Fix Go module cache mount path (cache currently unused).Builder sets GOMODCACHE=/app/.gopath/pkg/mod; compose mounts /go/pkg/mod. Mount to the path Go actually uses.
- - go_mod_cache:/go/pkg/mod + - go_mod_cache:/app/.gopath/pkg/modAlso applies to: 93-94
♻️ Duplicate comments (1)
main.go (1)
18-29: Sentry flush after capture is correctly handled.You now flush after CaptureException before os.Exit(1); defer covers the success path. Previous concern resolved.
🧹 Nitpick comments (5)
docker-compose.yml (1)
115-154: Add a stop_grace_period to match graceful server shutdown.RunServer uses a 10s shutdown window; give the container time to exit cleanly.
api: + stop_grace_period: 20spkg/endpoint/server.go (3)
59-61: Use structured attr for error field.Keep slog usage consistent and typed.
- if closeErr := server.Close(); closeErr != nil { - slog.Error("force close server failed", slog.String("address", addr), "error", closeErr) - } + if closeErr := server.Close(); closeErr != nil { + slog.Error("force close server failed", slog.String("address", addr), slog.Any("error", closeErr)) + }
118-124: Include PATCH and HEAD in allowed methods for typical REST usage.Common clients send HEAD; PATCH is frequently used for partial updates.
- AllowedMethods: []string{http.MethodGet, http.MethodPost, http.MethodPut, http.MethodDelete, http.MethodOptions}, + AllowedMethods: []string{http.MethodGet, http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete, http.MethodOptions, http.MethodHead},
113-116: DevHost likely not a valid browser Origin; sanitize or rename to DevOrigin.Main passes addr (e.g., 0.0.0.0:PORT). Browsers never send that as Origin. Either pass a proper origin (http://localhost:PORT) or normalize here.
Option A (preferred): Change the config field to DevOrigin and pass a full origin.
Option B: Normalize defensively:
@@ - if host := cfg.DevHost; host != "" { - origins = append(origins, host) - } + if host := cfg.DevHost; host != "" { + // best-effort normalization for addr-like inputs + if !strings.HasPrefix(host, "http://") && !strings.HasPrefix(host, "https://") { + host = "http://" + host + } + if !strings.Contains(host, "0.0.0.0") { + origins = append(origins, host) + } + }And add:
+ "strings"Please confirm what env.Network.GetHostURL() returns.
main.go (1)
60-62: Avoid shadowing the sentry import.Minor readability nit; use a different local name.
- if sentry := app.GetSentry(); sentry != nil && sentry.Handler != nil { - wrap = sentry.Handler.Handle + if s := app.GetSentry(); s != nil && s.Handler != nil { + wrap = s.Handler.Handle
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docker-compose.yml(1 hunks)docker/dockerfile-api(1 hunks)main.go(1 hunks)pkg/endpoint/server.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
main.go (4)
pkg/portal/validator.go (1)
GetDefaultValidator(23-33)metal/kernel/ignite.go (1)
Ignite(11-17)metal/kernel/app.go (1)
NewApp(26-42)pkg/endpoint/server.go (3)
NewServerHandler(89-134)ServerHandlerConfig(78-83)RunServer(20-74)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test (1.25.3)
gocanto
deleted the
gus/refactor-code-to-be-more-idiomatic-go-2025-10-17
branch
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68f19e2795d8833382f305f172362af1
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Tests