feat: start on backend #1

Merged

Pertempto merged 8 commits from 0-backend-creation into main 2025-11-20 16:43:13 -05:00

Conversation 4 Commits 8 Files changed 13 +353 -25

Pertempto commented 2025-11-20 16:06:22 -05:00 (Migrated from github.com)

Copy link

No description provided.

github-actions[bot] commented 2025-11-20 16:10:02 -05:00 (Migrated from github.com)

Copy link

Changes Requested

• Make the HTTP listen address configurable (env var HTTP_ADDR or CLI flag) with default :8080 and use it in backend/cmd/server/main.go instead of hardcoding.
• Update backend/internal/config/config.go to (a) join missing env keys with ", " for readability, and (b) include an optional HTTP_ADDR setting (or clearly document that main provides it). Ensure .env.example documents HTTP_ADDR.
• Clarify the Server.Run doc comment to say it blocks until the provided ctx is canceled (signals) and that graceful shutdown uses a 10s timeout; consider renaming to Serve (optional).

Summary of Changes

• Added backend scaffold: backend/cmd/server/main.go with slog and signal-backed context.
• Implemented Server in backend/internal/server/server.go with route registration and /health handler; graceful shutdown and a 10s timeout implemented.
• Added env loader backend/internal/config/config.go using godotenv and collating missing env vars into a single error.
• Added backend/go.mod, backend/go.sum, .env.example, updated CI workflow, and docs updates (CONTRIBUTING.md, specs, etc.).

Overall Feedback

• Negative: The current change hardcodes the HTTP listen address and uses a terse missing-vars message. Before merging, please make the listen address configurable (env var HTTP_ADDR or a small flag), surface HTTP_ADDR in .env.example or in Settings, and make the missing-vars join use ", " so errors are easy to read.

• Positive: The backend scaffold is well-structured. Use of slog, signal-backed context, and a clear server abstraction with graceful shutdown are strong foundations. CI and docs updates are thoughtful and helpful — nice work! 🎉

Please address the three requested changes and push an update; I'll re-review the patch once those items are resolved. Thanks!

# Changes Requested - Make the HTTP listen address configurable (env var `HTTP_ADDR` or CLI flag) with default `:8080` and use it in `backend/cmd/server/main.go` instead of hardcoding. - Update `backend/internal/config/config.go` to (a) join missing env keys with ", " for readability, and (b) include an optional `HTTP_ADDR` setting (or clearly document that `main` provides it). Ensure `.env.example` documents `HTTP_ADDR`. - Clarify the `Server.Run` doc comment to say it blocks until the provided `ctx` is canceled (signals) and that graceful shutdown uses a 10s timeout; consider renaming to `Serve` (optional). ## Summary of Changes - Added backend scaffold: `backend/cmd/server/main.go` with `slog` and signal-backed context. - Implemented `Server` in `backend/internal/server/server.go` with route registration and `/health` handler; graceful shutdown and a 10s timeout implemented. - Added env loader `backend/internal/config/config.go` using `godotenv` and collating missing env vars into a single error. - Added `backend/go.mod`, `backend/go.sum`, `.env.example`, updated CI workflow, and docs updates (CONTRIBUTING.md, specs, etc.). ## Overall Feedback - Negative: The current change hardcodes the HTTP listen address and uses a terse missing-vars message. Before merging, please make the listen address configurable (env var `HTTP_ADDR` or a small flag), surface `HTTP_ADDR` in `.env.example` or in `Settings`, and make the missing-vars join use ", " so errors are easy to read. - Positive: The backend scaffold is well-structured. Use of `slog`, signal-backed context, and a clear server abstraction with graceful shutdown are strong foundations. CI and docs updates are thoughtful and helpful — nice work! 🎉 Please address the three requested changes and push an update; I'll re-review the patch once those items are resolved. Thanks!

github-actions[bot] commented 2025-11-20 16:15:09 -05:00 (Migrated from github.com)

Copy link

@Pertempto — backend/cmd/server/main.go: please wire up signal-based shutdown so Run can exit via a canceled context instead of context.Background() which blocks forever.

Actionable change (suggestion): use signal.NotifyContext (or os/signal + context.WithCancel) and pass that ctx into srv.Run. Minimal example:

ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM)
defer stop()
if err := srv.Run(ctx, ":8080"); err != nil {
    logger.Error("server failed", slog.Any("error", err))
    os.Exit(1)
}

Also consider making the listen address configurable (env var or flag) instead of hardcoding :8080.

@Pertempto — backend/cmd/server/main.go: please wire up signal-based shutdown so `Run` can exit via a canceled context instead of `context.Background()` which blocks forever. Actionable change (suggestion): use `signal.NotifyContext` (or `os/signal` + `context.WithCancel`) and pass that ctx into `srv.Run`. Minimal example: ``` ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM) defer stop() if err := srv.Run(ctx, ":8080"); err != nil { logger.Error("server failed", slog.Any("error", err)) os.Exit(1) } ``` Also consider making the listen address configurable (env var or flag) instead of hardcoding `:8080`.

github-actions[bot] commented 2025-11-20 16:15:32 -05:00 (Migrated from github.com)

Copy link

File: backend/internal/server/server.go

• Actionable change: handle the w.Write error in the /health handler (currently ignored). At minimum, check the return value and log it. Example inside a handler with access to a logger:

w.Header().Set("Content-Type", "application/json")
w.WriteHeader(http.StatusOK)
if _, err := w.Write([]byte(`{"status":"ok"}`)); err != nil {
    // log the error so it isn't silently dropped
}

• Suggestion: if you want to log write errors, move route registration into the Server receiver (e.g. func (s *Server) registerRoutes()) so handlers can use s.logger for error logging. This makes the health handler more robust and debuggable.

Tagging @Pertempto for visibility.

File: `backend/internal/server/server.go` - Actionable change: handle the `w.Write` error in the `/health` handler (currently ignored). At minimum, check the return value and log it. Example inside a handler with access to a logger: ``` w.Header().Set("Content-Type", "application/json") w.WriteHeader(http.StatusOK) if _, err := w.Write([]byte(`{"status":"ok"}`)); err != nil { // log the error so it isn't silently dropped } ``` - Suggestion: if you want to log write errors, move route registration into the `Server` receiver (e.g. `func (s *Server) registerRoutes()`) so handlers can use `s.logger` for error logging. This makes the health handler more robust and debuggable. Tagging @Pertempto for visibility.

github-actions[bot] commented 2025-11-20 16:36:51 -05:00 (Migrated from github.com)

Copy link

File: backend/internal/config/config.go

• Good: you already collect missing env vars and return a single error listing them.
• Request: please include HTTP_ADDR (or ADDR) as an optional config key or document that main provides defaults. If you prefer to keep HTTP_ADDR out of Settings, explicitly note in the comment that Load only validates DB envs.
• Suggestion: consider using a small helper to join the missing keys with ", " for readability (e.g. strings.Join(missing, ", ")) — currently you use "," as the separator which yields no space between names.

File: `backend/internal/config/config.go` - Good: you already collect missing env vars and return a single error listing them. - Request: please include `HTTP_ADDR` (or `ADDR`) as an optional config key or document that `main` provides defaults. If you prefer to keep `HTTP_ADDR` out of `Settings`, explicitly note in the comment that `Load` only validates DB envs. - Suggestion: consider using a small helper to join the missing keys with `", "` for readability (e.g. `strings.Join(missing, ", ")`) — currently you use `","` as the separator which yields no space between names.

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

kwila/office-automations!1

No description provided.