feat(frontend): Go web server skeleton + /healthz + Dockerfile#35
Conversation
Summary of ChangesHello, 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 establishes the foundation for the project's frontend container. It provides a lightweight Go skeleton designed to be deployable and probe-able, ensuring parity with the existing backend architecture. This setup allows the CI/CD pipeline to successfully build and publish both containers, while leaving room for future UI and API integration work. 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 the 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 counterproductive. 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. 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 introduces the initial skeleton for the Go-based frontend service, including a multi-stage Dockerfile, a chi router with a /healthz endpoint, and basic unit tests. The review feedback suggests improving efficiency by caching static build information at startup, ensuring logging consistency by using slog-compatible middleware, and avoiding a potential race condition by setting up signal handling before the server starts.
| func healthzHandler(w http.ResponseWriter, _ *http.Request) { | ||
| info := BuildInfo{ | ||
| Status: "ok", | ||
| Version: envDefault("HUB_VERSION", "0.0.0-dev"), | ||
| Revision: envDefault("HUB_REVISION", "unknown"), | ||
| BuildDate: envDefault("HUB_BUILD_DATE", "1970-01-01T00:00:00Z"), | ||
| Repository: envDefault("HUB_REPO_URL", defaultRepoURL), | ||
| } |
There was a problem hiding this comment.
Environment variables like HUB_VERSION and HUB_REVISION are static and do not change during the process lifetime. Reading them from the environment on every request to /healthz is inefficient. Consider populating the BuildInfo struct once at startup and reusing it in the handler.
There was a problem hiding this comment.
Fixed in c60900e. HUB_VERSION/HUB_REVISION/HUB_BUILD_DATE/HUB_REPO_URL are now read once at startup via loadBuildInfo() and cached in a package-level buildInfo struct; /healthz serves that cached value instead of hitting os.Getenv per request. The container can't change these at runtime, so a one-time read is correct and removes per-request syscalls on a hot probe endpoint.
| r.Use(middleware.RequestID) | ||
| r.Use(middleware.RealIP) | ||
| r.Use(middleware.Recoverer) | ||
| r.Use(middleware.Logger) |
There was a problem hiding this comment.
The project uses slog for application logging, but middleware.Logger from chi uses the standard log package. This results in inconsistent log formats (e.g., if slog is later configured for JSON output). Consider using a middleware that integrates with slog to maintain consistency across all logs.
There was a problem hiding this comment.
Fixed in c60900e. Replaced middleware.Logger with a small custom slogRequestLogger that wraps the response writer (middleware.NewWrapResponseWriter) to capture status/bytes and emits one structured slog.Info per request with method/path/status/bytes/duration_ms/request_id/remote_ip. This way request logs honour whatever slog handler we configure later (level, format, destination) instead of going through stdlib log.
| idleConnsClosed := make(chan struct{}) | ||
| go func() { | ||
| sigCh := make(chan os.Signal, 1) | ||
| signal.Notify(sigCh, syscall.SIGTERM, syscall.SIGINT) | ||
| <-sigCh | ||
| slog.Info("shutdown signal received") |
There was a problem hiding this comment.
The signal notification should be set up before starting the goroutine and the server. This ensures that any signals received during the application's startup phase are correctly captured and handled, avoiding a potential race condition where a signal might be missed if it arrives before signal.Notify is called.
| idleConnsClosed := make(chan struct{}) | |
| go func() { | |
| sigCh := make(chan os.Signal, 1) | |
| signal.Notify(sigCh, syscall.SIGTERM, syscall.SIGINT) | |
| <-sigCh | |
| slog.Info("shutdown signal received") | |
| sigCh := make(chan os.Signal, 1) | |
| signal.Notify(sigCh, syscall.SIGTERM, syscall.SIGINT) | |
| idleConnsClosed := make(chan struct{}) | |
| go func() { | |
| <-sigCh | |
| slog.Info("shutdown signal received") |
There was a problem hiding this comment.
Fixed in c60900e. signal.Notify(sigCh, syscall.SIGTERM, syscall.SIGINT) is now called before the shutdown goroutine launches and before srv.ListenAndServe(). The buffered channel is captured by the goroutine via closure, so any signal arriving in the launch-window is queued instead of triggering the default terminate-by-signal behaviour.
There was a problem hiding this comment.
Pull request overview
Adds an initial Go-based frontend service skeleton under frontend/ so the repository has a buildable/publishable second container (matching the backend’s /healthz probe shape and intended docker-publish workflow integration).
Changes:
- Introduces a minimal chi-based Go HTTP server with
/healthz, graceful shutdown, and unit tests. - Adds a multi-stage Dockerfile for a non-root Alpine runtime image with healthcheck and build metadata env vars.
- Adds frontend module scaffolding (
go.mod/go.sum),.dockerignore, and a small frontend README.
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/README.md | Documents local dev and runtime configuration for the frontend skeleton. |
| frontend/go.mod | Creates a Go module for the frontend and pins chi v5. |
| frontend/go.sum | Adds checksums for chi dependency. |
| frontend/Dockerfile | Multi-stage build producing a non-root runtime image with /healthz healthcheck and build metadata plumbing. |
| frontend/cmd/server/main.go | Implements the chi router + /healthz endpoint and graceful shutdown. |
| frontend/cmd/server/main_test.go | Adds unit tests for /healthz and envDefault. |
| frontend/.dockerignore | Minimizes Docker build context for the frontend image. |
| func TestEnvDefault_UsesEnvWhenSet(t *testing.T) { | ||
| t.Parallel() | ||
| t.Setenv("FRONTEND_TEST_KEY", "expected") | ||
| got := envDefault("FRONTEND_TEST_KEY", "fallback") | ||
| if got != "expected" { | ||
| t.Errorf("envDefault returned %q, want expected", got) | ||
| } |
| func TestEnvDefault_UsesFallbackWhenEmpty(t *testing.T) { | ||
| t.Parallel() | ||
| t.Setenv("FRONTEND_EMPTY_KEY", "") | ||
| got := envDefault("FRONTEND_EMPTY_KEY", "fallback") | ||
| if got != "fallback" { | ||
| t.Errorf("envDefault with empty env returned %q, want fallback", got) | ||
| } |
There was a problem hiding this comment.
Fixed in ff9afaf — t.Parallel() removed from this test as well. Same reasoning as the comment above.
| Handler: r, | ||
| ReadHeaderTimeout: 10 * time.Second, | ||
| ReadTimeout: 30 * time.Second, | ||
| WriteTimeout: 30 * time.Second, |
There was a problem hiding this comment.
Fixed in c60900e. WriteTimeout is now 0 (no deadline) with an inline comment explaining why: the frontend will proxy Server-Sent Events from the backend, and any non-zero WriteTimeout would tear down a long-lived SSE response mid-stream. Per-route timeouts for non-SSE routes will be added once those routes exist (chi Timeout middleware applied per-route).
| # Build-time arguments — set by the CI workflow (.github/workflows/docker-publish.yml). | ||
| # Local builds without --build-arg get the placeholder defaults below. | ||
| ARG VERSION=0.0.0-dev | ||
| ARG REVISION=unknown | ||
| ARG BUILD_DATE=1970-01-01T00:00:00Z | ||
|
|
||
| # OCI image labels — visible via `docker inspect`. Keep in sync with the |
There was a problem hiding this comment.
Already resolved by the rebase onto main. .github/workflows/docker-publish.yml now passes VERSION/REVISION/BUILD_DATE as build-args (the workflow was updated together with the backend container in #34, and the rebase brought those changes into this branch). You can verify by looking at the workflow on feat/frontend-container — the Build & push (multi-arch) step includes:
build-args: |
VERSION=${{ steps.tag.outputs.tag }}
REVISION=${{ github.sha }}
BUILD_DATE=${{ steps.builddate.outputs.date }}Released images will therefore have populated HUB_VERSION/HUB_REVISION/HUB_BUILD_DATE ENV vars and matching OCI labels.
| | `HUB_VERSION` | `0.0.0-dev` | Baked in by the Dockerfile from the release tag | | ||
| | `HUB_REVISION` | `unknown` | Baked in by the Dockerfile from the git SHA | | ||
| | `HUB_BUILD_DATE` | `1970-01-01T00:00:00Z` | Baked in by the Dockerfile at build time | | ||
| | `HUB_REPO_URL` | `https://github.com/strausmann/label-printer-hub` | Baked in by the Dockerfile | |
There was a problem hiding this comment.
Same as the Dockerfile comment above — the workflow gap was closed by the rebase onto main. docker-publish.yml passes VERSION/REVISION/BUILD_DATE as build-args, so the README claim that HUB_* are baked in from the release tag/SHA/build time is accurate for published images.
First container-buildable code for the frontend. Mirrors the backend's
shape (chi router with /healthz, multi-stage Dockerfile, OCI labels,
non-root UID 1000, configurable PORT, multi-arch buildable) so the
docker-publish workflow can push both images side-by-side at the next
release.
Go skeleton (cmd/server/main.go):
- chi router with RequestID + RealIP + Logger + Recoverer middleware
- /healthz returns the same JSON shape as the backend: status, version,
revision, build_date, repository — so orchestrator probe configs
work for both containers uniformly
- BuildInfo populated from HUB_* env vars baked in by Dockerfile;
safe defaults when running uncontained (local dev, unit tests)
- Graceful shutdown on SIGTERM/SIGINT with 15s context timeout
- ReadHeaderTimeout / ReadTimeout / WriteTimeout / IdleTimeout all
set to sane defaults (mitigates slowloris-style attacks)
Tests (8 unit tests):
- /healthz: 200, correct JSON shape, Content-Type, no auth required
- /healthz body does not leak secret-ish substrings (parity with
backend test_does_not_expose_secrets)
- envDefault: fallback on unset / empty, env value when set
Dockerfile (multi-stage):
- Stage 1 (builder, golang:1.23-alpine): go mod download cached
separately, then CGO_ENABLED=0 static binary with -trimpath -ldflags
'-s -w' for smallest reproducible output
- Stage 2 (runtime, alpine:3.20): tini + curl (HEALTHCHECK) +
ca-certificates (HTTPS to backend for proxied requests later) +
non-root UID 1000 user matching the backend container
- ARGs VERSION / REVISION / BUILD_DATE flow into OCI labels (12 of them,
identical schema to backend) AND runtime ENV vars (HUB_VERSION,
HUB_REVISION, HUB_BUILD_DATE, HUB_REPO_URL) so the running app
surfaces them through /healthz
- Shell-form HEALTHCHECK expands ${PORT:-8080}
- ENTRYPOINT tini -- /usr/local/bin/server (no shell wrapper needed;
Go binary reads $PORT itself)
.dockerignore minimal: ignores build output, IDE, secrets, .git,
node_modules (Tailwind toolchain lands later).
frontend/README.md: subdirectory README pointing at the repo root,
documenting env vars and the local dev loop.
Verified locally with --build-arg + docker run + curl /healthz on
default port (8080) and custom port (PORT=7777).
This is intentionally a SKELETON — Tailwind, HTMX, PWA manifest,
service worker, OpenAPI-generated backend client, and the actual UI
routes land in follow-up PRs once the backend exposes real endpoints.
The point of this PR is to make 'docker compose up' work end-to-end
with both containers from the next release.
Refs: ADR 0001 (two-container), ADR 0003 (Go + Tailwind + HTMX + PWA),
ADR 0007 (multi-arch tag scheme)
Go's testing package panics when a test uses both t.Parallel() and t.Setenv (or t.Chdir, or cryptotest.SetGlobalRandom). The setenv calls mutate process-wide state and can't safely interleave with parallel tests. The two affected tests in main_test.go set FRONTEND_TEST_KEY and FRONTEND_EMPTY_KEY via t.Setenv — they now run serially, while the six other tests still use t.Parallel(). Caught by the Go CI job on PR #35. Adding this pattern to docs/learnings/code-review-patterns.md in a follow-up commit if it likely recurs — Go contributors may not know the t.Setenv/t.Parallel incompatibility off the top of their head.
d2c2add to
ff9afaf
Compare
…imeout Addresses Gemini + Copilot review findings on PR #35: - Cache HUB_* env vars once at startup into `buildInfo` instead of reading os.Getenv on every /healthz request (Gemini): the values are baked into the image and never change at runtime, so per-request syscalls were waste. - Replace chi's `middleware.Logger` with a small custom slog-based request logger (Gemini): chi's logger writes through the stdlib `log` package and bypasses our slog handler, so request lines would not honour the configured log level/format/destination. - Register signal.Notify BEFORE launching the shutdown goroutine (Gemini): a SIGTERM arriving in the scheduling window between `go func` and the channel registration would otherwise terminate the process by default instead of triggering graceful shutdown. - Set `WriteTimeout: 0` with explanatory comment (Copilot): the frontend will proxy Server-Sent Events, and any non-zero WriteTimeout would tear down long-lived SSE responses mid-stream. Per-route timeouts will be applied to non-SSE routes when they are added. Tests: - New TestLoadBuildInfo_AppliesEnvOverrides verifies the startup-cache path. - New TestLoadBuildInfo_UsesDefaultsWhenUnset verifies the fallback path. - Existing tests now call initBuildInfoForTests so /healthz sees populated values (main() is what loads the cache in production, and that does not run during `go test`).
`go test -race` flagged a race on the global `buildInfo` var: every healthz test calls `initBuildInfoForTests`, and with `t.Parallel()` those calls run concurrently — multiple goroutines wrote to the same variable. Wrapping the write in `sync.Once` makes the initialization race-free: the first caller populates `buildInfo`, every subsequent caller is a no-op. Test correctness is preserved because `loadBuildInfo()` is pure with respect to the env it reads — once it has produced a value, calling it again with the same env would produce the same value.
Summary
Parallel companion to #34 — builds the second container so the docker-publish workflow has two real images to push at the next release. Mirrors the backend's shape (chi router, /healthz with the same JSON keys, OCI labels, non-root UID 1000, configurable PORT, multi-arch).
This is intentionally a skeleton. The actual UI — Tailwind CSS pipeline, HTMX templates, PWA manifest + service worker, OpenAPI-generated backend client (per ADR 0011) — lands in follow-up PRs once the backend exposes real endpoints. The point of this PR is "both containers exist, both publish, both probe-able".
What's in this PR
Go skeleton
Tests (8 unit tests, Go `testing`)
Dockerfile (multi-stage)
`.dockerignore` and `README.md`
Minimal context: ignores build output, secrets, .git, node_modules (Tailwind toolchain lands in a follow-up PR). README documents env vars + local dev loop, points at the repo root for project-level docs.
Verified locally
Linked issue
(maintainer chose 'Weg A' — minimal skeleton now, real UI later)
Type of change
Hardware tested on
Test coverage
Checklist
Coordination with #34