gsadmin/certctl
1
0
Fork 0
mirror of https://github.com/shankar0123/certctl.git synced 2026-06-07 12:21:31 +00:00
Code Issues Packages Projects Releases Wiki Activity

refactor(cmd/server): extract migration block to migrations.go (Phase 9, 8b — behavior-aware)

Browse Source 
Closes the third file Sprint 8 deferred. Sprint 8a (commit 3f1344e8)
shipped the pure-mechanical relocation of wire.go (helpers + adapter
types). Sprint 8b crosses the behavior-change boundary: extracts an
inline block from main()'s body into a new function, which introduces
a new function call frame.

What moved
==========
  cmd/server/migrations.go (new, 209 lines incl. BSL header + Phase 9
                           doc-comment + 6 imports + 2 functions)

Two unexported helpers:
  - parseMigrateOnlyFlag() bool — hand-parses os.Args[1:] for the
    `--migrate-only` token. Six-line implementation; matches the
    pre-Sprint-8b inline behavior exactly (bare match, no value form,
    no env override). Hand-parsed (not flag.Parse) for the same
    reason the original was: keeps flag.Parse's global state out of
    package main so future imports stay clean.
  - runBootMigrations(cfg, db, logger, migrateOnly) bool — owns the
    Phase 4 DEPL-M1 migration-via-hook posture. Reads
    CERTCTL_MIGRATIONS_VIA_HOOK, gates RunMigrations + RunSeed,
    handles the --migrate-only early-exit signal, runs RunDemoSeed
    when CERTCTL_DEMO_SEED=true.
    Returns true ONLY when migrateOnly was set; caller (main)
    handles the clean exit via `return` so deferred cleanup runs.
    Returns false in every other case — caller continues normal boot.
    On any migration / seed error: os.Exit(1) inline (matches the
    pre-extraction shape; recovery is impossible at this boot stage).

main.go delta
=============
  - Lines 54-72 (the --migrate-only flag parse + its Phase 4
    doc-comment): replaced with a single call
    `migrateOnly := parseMigrateOnlyFlag()` plus a 6-line pointer
    to migrations.go.
  - Lines 178-259 (the migrations-via-hook + RunMigrations +
    RunSeed + --migrate-only early-exit + RunDemoSeed inline
    block): replaced with a single call
    `if exitAfterMigrations := runBootMigrations(cfg, db, logger,
    migrateOnly); exitAfterMigrations { return }` plus an 8-line
    pointer to migrations.go.
  - No imports needed adjusting in main.go — the moved code's
    imports (database/sql, strings) were ALSO used by the rest of
    main(); they stay. (Notably, this is unlike Sprint 8a, which
    surfaced 5 unused imports requiring removal.)

main.go LOC: 2347 → 2260  (-87 lines)

Behavior-change contract (the single intentional shift)
========================================================
Every error path inside runBootMigrations calls os.Exit(1) directly
— byte-for-byte equivalent to the original inline shape (same log
message, same exit code, same no-defer-run on fatal).

THE ONE BEHAVIOR CHANGE: the --migrate-only SUCCESS path now returns
to main() rather than calling os.Exit(0) inline. Observable effect:
the `defer db.Close()` registered at line 175 in main() now runs at
clean exit instead of being skipped.

Why this is strictly an improvement (not a regression):
  - The original os.Exit(0) skipped every registered defer. db.Close
    never ran; the OS reclaimed the socket when the process died.
  - The new `return` causes db.Close to run on the orderly main()
    teardown path. PostgreSQL connection released cleanly via the
    Go *sql.DB.Close() contract rather than mid-flight socket
    teardown.
  - Migrations + seed are SYNCHRONOUS — by the time runBootMigrations
    returns true, all SQL work has fsync'd or returned errors. There's
    no async work that db.Close could truncate.
  - The exit code stays 0 (Kubernetes Job lifecycle still reports
    success).
  - The exit log message ("--migrate-only: migrations + seed
    complete; exiting without starting server lifecycle") fires
    BEFORE the return, identical to the pre-extraction position.

If an operator's monitoring is wired to detect "did the --migrate-only
container clean-shutdown its DB connection or did it just die," they
will see the new behavior. Every other observable signal is identical.

Documented in migrations.go's doc-comment so the next maintainer
doesn't think the change was accidental.

Why this is a separate commit from Sprint 8a
============================================
Sprint 8a was pure mechanical relocation — function definitions
moved between sibling files in the same package, zero runtime
semantics changed. Sprint 8b introduces a new function call frame,
which has a non-zero (if small + documented + improvement-shaped)
behavior delta.

Splitting these into two commits means git bisect against a future
boot-time regression gets a clean answer:
  3f1344e8 ... wire.go        — could not have changed behavior
  <this>   ... migrations.go  — one specific documented shift, see
                                commit body + migrations.go header

Anyone tracing a boot-time issue knows EXACTLY which commit to scrutinize.

Verification (all clean):
  go build ./cmd/server/...               → clean (no unused imports)
  go vet ./cmd/server/...                 → clean
  gofmt -l cmd/server/                    → clean
  go test ./cmd/server/... -count=1 -short → ok (0.39s; main_test.go
                                              + the existing
                                              preflight_*_test.go +
                                              finalhandler_test.go +
                                              auth_*_test.go +
                                              tls_test.go all pass —
                                              including main_test.go
                                              which exercises the
                                              boot flow through the
                                              new call site)
  staticcheck ./cmd/server/...            → clean
  grep -nE 'migrateOnly|migrationsViaHook|RunMigrations|RunSeed|RunDemoSeed'
       cmd/server/main.go   → just the runBootMigrations call site +
                                the parseMigrateOnlyFlag call site;
                                the inline block is gone.

LOC delta:
  main.go:       2347 → 2260  (-87 lines: -18 from flag-parse
                                          extraction, -75 from
                                          migration-block extraction,
                                          +6 from new call-site +
                                          pointer comments)
  migrations.go: new, 209 lines (incl. ~95-line Phase 9 doc-comment +
                                BSL header + package decl + 6-line
                                import block)

Phase 9 Sprint 8 closure
========================
Sprint 8a (wire.go) + Sprint 8b (this commit) together close the
Phase 9 prompt's three-file split for cmd/server/main.go:

  cmd/server/main.go        2966 → 2260  (-706 lines, -23.8%)
  cmd/server/wire.go        new, 758 LOC
  cmd/server/migrations.go  new, 209 LOC

Cumulative Phase 9 (Sprints 1-8b):
  config.go:                 3403 → 1342 LOC (-60.6% across 7 sprints)
  cmd/server/main.go:        2966 → 2260 LOC (-23.8% across this
                                              sprint + Sprint 8a)
  Combined LOC reduction in the two largest backend files: -2,767

Next queued (Sprint 9): internal/service/acme.go (1965 LOC). Per
the operator's decision after Sprint 8 (Option B = sibling files
in the same package, no subpackage split): the cut will keep the
package name `service` and split into
internal/service/{acme,acme_orders,acme_authz,acme_challenges,
acme_nonces,acme_gc}.go. Zero import-path churn for callers.

Closes: cowork/certctl-architecture-diligence-audit.html#fix-ARCH-M2
        (partial — Sprint 8 fully closed at 9 of 12 effective splits)
This commit is contained in:
shankar0123
2026-05-14 09:13:38 +00:00
parent 3f1344e806
commit de4f93b35e
2 changed files with 222 additions and 99 deletions
Download Patch File Download Diff File Expand all files Collapse all files
cmd/server/main.go
+13 -99
View File
@@ -52,24 +52,11 @@ import (
func main() {
// Phase 4 DEPL-M1 closure (2026-05-14): --migrate-only flag for
// the Helm pre-install/pre-upgrade hook (see
// deploy/helm/certctl/templates/migration-job.yaml). When set, the
// server loads config, opens the DB pool, runs migrations + seed,
// and exits — no HTTP listener, no scheduler, no signing work.
// Same migration code path as boot-time RunMigrations; only the
// surrounding lifecycle differs.
//
// Hand-parsed (instead of pulling in flag.Parse) because the rest
// of the server's config surface is env-var driven via
// config.Load(); adding a flag.Parse() with global state risks
// conflicting with other binaries that import cmd/server later.
migrateOnly := false
for _, arg := range os.Args[1:] {
if arg == "--migrate-only" {
migrateOnly = true
break
}
}
// the Helm pre-install/pre-upgrade hook. Phase 9 Sprint 8b
// (2026-05-14) extracted the flag-parse + the migration-execution
// block to cmd/server/migrations.go; see that file's doc-comment
// for the full Phase 4 lifecycle rationale.
migrateOnly := parseMigrateOnlyFlag()
// Load configuration
cfg, err := config.Load()
@@ -175,87 +162,14 @@ func main() {
defer db.Close()
logger.Info("connected to database")
// Phase 4 DEPL-M1 closure (2026-05-14): migration-via-hook posture.
//
// Three lifecycles to support:
// (a) Compose / VM / bare-metal: server runs migrations at boot.
// Default behavior — preserved unchanged.
// (b) Helm with pre-install/pre-upgrade hook: the migration Job
// runs `certctl-server --migrate-only`, does its work, and
// exits. The server Deployment's pods then start with
// CERTCTL_MIGRATIONS_VIA_HOOK=true set; they see the env
// var and skip their boot-time RunMigrations call so the
// Job's work isn't duplicated.
// (c) Bare `certctl-server --migrate-only` invocation (e.g.
// operator running a one-shot migration from the CLI):
// runs migrations + seed and exits cleanly. No HTTP
// listener, no scheduler, no signing work.
//
// migrateOnly captures case (c); CERTCTL_MIGRATIONS_VIA_HOOK
// captures case (b). Both paths converge on the same RunMigrations
// + RunSeed code below.
migrationsViaHook := strings.EqualFold(os.Getenv("CERTCTL_MIGRATIONS_VIA_HOOK"), "true")
if migrateOnly || !migrationsViaHook {
logger.Info("running migrations", "path", cfg.Database.MigrationsPath)
if err := postgres.RunMigrations(db, cfg.Database.MigrationsPath); err != nil {
logger.Error("failed to run migrations", "error", err)
os.Exit(1)
}
logger.Info("migrations completed")
} else {
logger.Info("skipping migrations at boot (CERTCTL_MIGRATIONS_VIA_HOOK=true — Helm pre-install/pre-upgrade hook owns this work)")
}
// Apply baseline seed data.
//
// U-3 (P1, cat-u-seed_initdb_schema_drift): pre-U-3 seed.sql was mounted
// into postgres `/docker-entrypoint-initdb.d/` alongside a hand-curated
// subset of migrations. Adding a migration that introduced a new column
// referenced by seed.sql (cat-o-retry_interval_unit_mismatch /
// policy_rules.severity / etc.) without also updating the compose volume
// mounts caused initdb to crash on first up. Post-U-3 the compose stack
// drops all initdb mounts; postgres comes up with empty schema, the
// server runs RunMigrations above, then this RunSeed call lands the
// baseline data — all from a single source of truth (this binary).
// See internal/repository/postgres/db.go::RunSeed for the contract.
//
// Phase 4 DEPL-M1: same migration-via-hook gating as RunMigrations.
// When the hook owns migrations it also owns the seed pass.
if migrateOnly || !migrationsViaHook {
logger.Info("applying baseline seed", "path", cfg.Database.MigrationsPath)
if err := postgres.RunSeed(db, cfg.Database.MigrationsPath); err != nil {
logger.Error("failed to apply seed data", "error", err)
os.Exit(1)
}
logger.Info("seed completed")
} else {
logger.Info("skipping baseline seed at boot (CERTCTL_MIGRATIONS_VIA_HOOK=true — hook applies seed alongside migrations)")
}
// Phase 4 DEPL-M1: --migrate-only early-exit. Migrations + seed are
// done; the operator only asked for the migration pass. Skip the
// HTTP listener, scheduler, signing setup, banner, etc. Exit 0
// cleanly so Kubernetes Job lifecycle reports success.
if migrateOnly {
logger.Info("--migrate-only: migrations + seed complete; exiting without starting server lifecycle")
os.Exit(0)
}
// Apply demo overlay seed when CERTCTL_DEMO_SEED=true. Pre-U-3 the demo
// overlay (deploy/docker-compose.demo.yml) mounted seed_demo.sql into
// postgres `/docker-entrypoint-initdb.d/`; that broke once U-3 dropped
// the initdb migration mounts (the demo seed references tables that
// wouldn't exist at initdb time). The runtime path here is the
// post-U-3 replacement. Default-off so a vanilla deploy never lands
// fake-history rows. See postgres.RunDemoSeed for the contract.
if cfg.Database.DemoSeed {
logger.Info("applying demo seed (CERTCTL_DEMO_SEED=true)", "path", cfg.Database.MigrationsPath)
if err := postgres.RunDemoSeed(db, cfg.Database.MigrationsPath); err != nil {
logger.Error("failed to apply demo seed data", "error", err)
os.Exit(1)
}
logger.Info("demo seed completed")
// Phase 4 DEPL-M1 + Phase 9 Sprint 8b — the migration-via-hook
// posture (Compose / Helm-with-hook / bare --migrate-only) lives
// in runBootMigrations (cmd/server/migrations.go). Returns true
// when --migrate-only was set so we can return from main()
// cleanly (deferred db.Close runs vs the pre-Sprint-8b os.Exit(0)
// which skipped defers — see migrations.go for the rationale).
if exitAfterMigrations := runBootMigrations(cfg, db, logger, migrateOnly); exitAfterMigrations {
return
}
// Initialize repositories with real PostgreSQL connection
cmd/server/migrations.go
+209
View File
@@ -0,0 +1,209 @@
// Copyright 2026 certctl LLC. All rights reserved.
// SPDX-License-Identifier: BUSL-1.1
package main
import (
"database/sql"
"log/slog"
"os"
"strings"
"github.com/certctl-io/certctl/internal/config"
"github.com/certctl-io/certctl/internal/repository/postgres"
)
// Phase 9 ARCH-M2 closure Sprint 8b (2026-05-14): the deferred half of
// Sprint 8. Extracts the boot-time migration handling from main()'s
// inline body into two unexported helpers. Different shape from
// Sprints 1-7 (data-type relocation) and from Sprint 8a (existing
// helper-function relocation) — this sprint crosses the
// behavior-change boundary Sprint 8 first identified.
//
// What lives here
// ===============
// parseMigrateOnlyFlag() bool
// Hand-parses os.Args for `--migrate-only` (NOT flag.Parse — the
// server's config surface is otherwise env-var driven via
// config.Load; introducing flag.Parse's global state risks
// conflicting with other binaries that may import cmd/server later).
//
// runBootMigrations(cfg, db, logger, migrateOnly) (exitNow bool)
// Owns the Phase 4 DEPL-M1 migration-via-hook posture: the
// migrationsViaHook env-var read, the RunMigrations + RunSeed
// gate, the --migrate-only early-exit signal, and the
// CERTCTL_DEMO_SEED demo-overlay branch.
//
// Returns true ONLY when --migrate-only was set and migrations +
// seed completed cleanly. The caller (main) translates that to
// `return` rather than os.Exit(0) — which is the SOLE intentional
// behavior change in this sprint (see below).
//
// Behavior preservation contract
// ==============================
// Every error path inside runBootMigrations calls os.Exit(1)
// directly, matching the original inline behavior byte-for-byte
// (same log message, same exit code, same no-defer-run-on-fatal
// semantics). The error-path os.Exit(1) is intentional: when
// migration fails at boot, the server cannot recover, and bailing
// out without running defers is the original Go-idiomatic shape.
//
// The ONE behavior change: the --migrate-only SUCCESS path now
// returns to main() rather than calling os.Exit(0) inline. This
// has one observable effect: the `defer db.Close()` registered in
// main() now runs at clean exit instead of being skipped. That's
// strictly better hygiene (clean DB connection shutdown vs OS
// reclaim). The migration work is synchronous + complete before
// the return; nothing async is left running that db.Close() could
// truncate.
//
// All other paths — the migration log messages, the seed log
// messages, the migrationsViaHook env-var read order, the
// RunDemoSeed gating, the per-step success/skip log lines — are
// byte-identical to the pre-Sprint-8b inline form. Verified via
// `go test ./cmd/server/... -count=1 -short` (which runs the
// existing main_test.go assertions through the new call site).
//
// Why this is a separate commit
// =============================
// Sprint 8a (commit see git log) extracted the bottom-of-file
// helpers + adapter types — pure mechanical relocation that
// couldn't change runtime semantics. Sprint 8b crosses the boundary
// where mechanical relocation ends: introducing a new function
// call frame changes defer scope, panic recovery, and (in this
// case) the exit semantics for the --migrate-only path. The
// Phase 9 prompt's "refactor is mechanical relocation; behavior
// change is a separate concern" rule guards against exactly this
// shape of risk being landed without a focused review.
//
// Splitting Sprint 8a (mechanical) from Sprint 8b (behavior-aware)
// means the operator's git log shows:
// 3f1344e8 ... wire.go — no behavior change possible
// <this> ... migrations.go — one specific behavior shift,
// documented + intentional
//
// Anyone bisecting a future bug to one of these two commits gets a
// clean "is it mechanical or did the behavior change" signal.
// parseMigrateOnlyFlag scans os.Args for the `--migrate-only` token
// and returns true if found. Hand-parsed instead of using flag.Parse
// because:
//
// 1. The server's entire config surface is env-var driven via
// config.Load(). flag.Parse() introduces a global package-state
// dependency that future binaries importing cmd/server (test
// harnesses, CLI tools, embedded variants) would have to
// coordinate around.
// 2. The only flag we care about is the migration-vs-server-lifecycle
// toggle; a hand-parser is 6 lines and has no transitive cost.
// 3. The flag is Helm-pre-install-hook-facing (see
// deploy/helm/certctl/templates/migration-job.yaml). Its shape is
// pinned by that template, not by anything else; we don't need
// flag.Parse's auto-help generation or type coercion.
//
// Bare arg match — no `=` value form, no short alias, no override
// from env. Anyone passing `--migrate-only` ANYWHERE in os.Args[1:]
// flips the flag on. Matches the original inline behavior exactly.
func parseMigrateOnlyFlag() bool {
for _, arg := range os.Args[1:] {
if arg == "--migrate-only" {
return true
}
}
return false
}
// runBootMigrations owns the Phase 4 DEPL-M1 boot-time migration
// posture. Three lifecycles to support:
//
// (a) Compose / VM / bare-metal: server runs migrations at boot.
// Default behavior — preserved unchanged.
// (b) Helm with pre-install/pre-upgrade hook: the migration Job
// runs `certctl-server --migrate-only`, does its work, and
// exits. The server Deployment's pods then start with
// CERTCTL_MIGRATIONS_VIA_HOOK=true set; they see the env
// var and skip their boot-time RunMigrations call so the
// Job's work isn't duplicated.
// (c) Bare `certctl-server --migrate-only` invocation (e.g.
// operator running a one-shot migration from the CLI):
// runs migrations + seed and returns true so main returns
// cleanly without starting the HTTP listener / scheduler /
// signing setup.
//
// migrateOnly captures case (c); CERTCTL_MIGRATIONS_VIA_HOOK
// captures case (b). Both paths converge on the same RunMigrations
// + RunSeed code below.
//
// Returns true ONLY when migrateOnly is set; caller (main) handles
// the clean exit via `return` so deferred cleanup (db.Close) runs.
// Returns false in every other case — caller continues normal boot.
// On any migration / seed error: os.Exit(1) inline (matches the
// pre-extraction shape; recovery is not possible at this boot
// stage).
func runBootMigrations(cfg *config.Config, db *sql.DB, logger *slog.Logger, migrateOnly bool) bool {
migrationsViaHook := strings.EqualFold(os.Getenv("CERTCTL_MIGRATIONS_VIA_HOOK"), "true")
if migrateOnly || !migrationsViaHook {
logger.Info("running migrations", "path", cfg.Database.MigrationsPath)
if err := postgres.RunMigrations(db, cfg.Database.MigrationsPath); err != nil {
logger.Error("failed to run migrations", "error", err)
os.Exit(1)
}
logger.Info("migrations completed")
} else {
logger.Info("skipping migrations at boot (CERTCTL_MIGRATIONS_VIA_HOOK=true — Helm pre-install/pre-upgrade hook owns this work)")
}
// Apply baseline seed data.
//
// U-3 (P1, cat-u-seed_initdb_schema_drift): pre-U-3 seed.sql was mounted
// into postgres `/docker-entrypoint-initdb.d/` alongside a hand-curated
// subset of migrations. Adding a migration that introduced a new column
// referenced by seed.sql (cat-o-retry_interval_unit_mismatch /
// policy_rules.severity / etc.) without also updating the compose volume
// mounts caused initdb to crash on first up. Post-U-3 the compose stack
// drops all initdb mounts; postgres comes up with empty schema, the
// server runs RunMigrations above, then this RunSeed call lands the
// baseline data — all from a single source of truth (this binary).
// See internal/repository/postgres/db.go::RunSeed for the contract.
//
// Phase 4 DEPL-M1: same migration-via-hook gating as RunMigrations.
// When the hook owns migrations it also owns the seed pass.
if migrateOnly || !migrationsViaHook {
logger.Info("applying baseline seed", "path", cfg.Database.MigrationsPath)
if err := postgres.RunSeed(db, cfg.Database.MigrationsPath); err != nil {
logger.Error("failed to apply seed data", "error", err)
os.Exit(1)
}
logger.Info("seed completed")
} else {
logger.Info("skipping baseline seed at boot (CERTCTL_MIGRATIONS_VIA_HOOK=true — hook applies seed alongside migrations)")
}
// Phase 4 DEPL-M1: --migrate-only early-exit. Migrations + seed are
// done; the operator only asked for the migration pass. Signal main
// to return cleanly so deferred db.Close runs (Sprint 8b improvement
// over the pre-extraction os.Exit(0) which skipped defers).
if migrateOnly {
logger.Info("--migrate-only: migrations + seed complete; exiting without starting server lifecycle")
return true
}
// Apply demo overlay seed when CERTCTL_DEMO_SEED=true. Pre-U-3 the demo
// overlay (deploy/docker-compose.demo.yml) mounted seed_demo.sql into
// postgres `/docker-entrypoint-initdb.d/`; that broke once U-3 dropped
// the initdb migration mounts (the demo seed references tables that
// wouldn't exist at initdb time). The runtime path here is the
// post-U-3 replacement. Default-off so a vanilla deploy never lands
// fake-history rows. See postgres.RunDemoSeed for the contract.
if cfg.Database.DemoSeed {
logger.Info("applying demo seed (CERTCTL_DEMO_SEED=true)", "path", cfg.Database.MigrationsPath)
if err := postgres.RunDemoSeed(db, cfg.Database.MigrationsPath); err != nil {
logger.Error("failed to apply demo seed data", "error", err)
os.Exit(1)
}
logger.Info("demo seed completed")
}
return false
}