Feat/database init cli - #24
Merged
Merged
Conversation
added 19 commits
August 15, 2026 15:31
Setup takes four prerequisites and a hand-edited 22-line .env, and provisioning has two entry points (nine make targets and boot-time bootstrap) with no obvious front door. Design replaces both with `oms database init|migrate|drop|status`, reimplemented on sqlx so psql and python3 stop being install requirements. Credentials follow NautilusTrader's model: CLI flag, then environment, then a working localhost default, which makes .env an override rather than a prerequisite. init is strict — it errors when the roles or database already exist rather than silently skipping or rotating passwords — and migrate is the idempotent verb for an existing install.
Ten tasks, each ending in an independently testable deliverable: config merge, embedded assets, provision, migrate, access+seed, orchestration, CLI wiring, removal of boot-time provisioning, deletion of make and the scripts, and CI that proves init is strict and migrate is idempotent.
Copies NautilusTrader's precedence order so a fresh clone works against a local Postgres with no configuration, and a deployment overrides only what it needs. This is what lets .env become an override file rather than a prerequisite.
Compiled in with include_dir so a released binary provisions without a repo checkout, which is what lets OMS_DB_SCRIPTS_DIR and the shell scripts go. The sort in migrations() is load-bearing: filename is the version.
Ports provision.sh. \gexec becomes an existence query plus a statement; CREATE ROLE and CREATE DATABASE are issued standalone because neither runs inside a transaction. inspect() is separate from provision() so init can refuse loudly on a server that already has our roles.
Same tracking table, targets and per-file transaction as migrate.sh, so an already-migrated database applies nothing. \i becomes reading an embedded file; SET ROLE is plain SQL, which is why psql is not needed.
Ports access.sh, seed.sh and seed_venues.py. The registry reshape becomes parse_mic_csv and the \copy-into-temp-table becomes one UNNEST upsert, which removes the python3 prerequisite. A changed ISO header now fails loudly instead of seeding an empty venue table.
SET ROLE / INSERT / RESET ROLE now run inside a single pool.begin() transaction (matching migrate.rs::apply_one), since three separate pool.execute() calls could each land on a different pooled connection — risking the INSERT running without mdm_master active, or a connection going back to the pool with mdm_master still set. Also strengthens the test fixture: MIC and OPERATING MIC now differ per row so a code/mic column swap would fail maps_registry_columns_onto_venue, and a new case covers the blank-name-falls-back-to-code path.
init is strict and says what to run instead; migrate is the idempotent verb for an existing install. Splitting the two is what lets init refuse without making re-runs impossible.
Starting the server no longer creates or migrates anything — oms database init is the only path, so provisioning logic cannot drift between two implementations. The runtime pool derives its connection from the same POSTGRES_* values init used and always connects as oms_user, which removes the DB_PASSWORD/OMS_USER_PASSWORD pair that had to agree by hand. Obsolete keys left in a .env are now reported by name with their replacement, because the alternative is a silent fallback to defaults.
Everything they did now lives in `oms database`. Prerequisites drop from Rust + Postgres + psql + python3 to Rust + Postgres, and there is one documented way to set up a clone instead of two. .env becomes an override file: every database value has a working localhost default, so a fresh checkout needs no configuration at all. Also fixes three stale message strings in src/preflight.rs left behind by the prior task: a `make db-seed` hint replaced with a file reference, the OMS_BOOTSTRAP doc comment (that env var is no longer read anywhere) rewritten to point at `oms database init`, and the sync-broker/db-fixtures hints updated to the current CLI subcommands.
Runs init on an empty Postgres, asserts a second init fails, and asserts a second migrate applies nothing — the two properties the CLI's contract rests on, neither of which can be tested without a server.
applying_twice_is_a_no_op asserted first-apply count >= 43, which only holds on a fresh database — it legitimately returns 0 once init/migrate already ran earlier in the same CI job. Drop that assertion; the test's real contract (second apply == 0, no pending after) is unaffected, and the exact count is already pinned by embeds_every_migration. check_removed_env_keys mixed bare replacement names and full prose under one hardcoded template, producing garbled messages like "use removed — the URL is built from POSTGRES_* now". Give each REMOVED_KEYS entry its own complete trailing clause instead.
The blocking one fell between tasks: setup::database_url() still read
DB_USER/DB_PASSWORD/DB_HOST/DB_PORT/DB_NAME — the five keys this branch
declares removed — so `oms setup sync-broker` and boot-time auto-sync both
died with "DB_USER must be set" on a fresh clone following the new README.
Every task passed its own review; nobody owned that file. The runtime URL is
now built in one place, PostgresConfig::runtime_url(), which serve() and the
setup subcommands share, and it percent-encodes the password so a role
password containing @ or / stops producing a malformed URL.
The rest are hardening the review asked for:
* init now applies the same default-password guard serve() had, so a
non-loopback provision cannot create roles with the shipped password —
previously the refusal only arrived later, at boot.
* a stale .env is fatal rather than advisory. The removed-key check was
the spec's only defence against connecting to the wrong database, and a
warning among dozens of log lines is not a defence.
* status no longer calls ensure_tracking. A read-only verb was issuing
CREATE SCHEMA, which against a mistyped POSTGRES_DATABASE would plant an
oms schema in someone else's database.
* drop requires --yes off-loopback; WITH (FORCE) terminates live sessions.
* refusals exit non-zero instead of returning, so a security stop does not
look like a clean shutdown to a supervisor.
* ADMIN_PASSWORD joins REMOVED_KEYS; its partner ADMIN_USER was already
there and nothing reads either any more.
Test hygiene: ENV_LOCK recovers from poisoning, so one genuine failure no
longer cascades into five, and clear_env() scrubs the removed keys too —
cargo test no longer depends on what the developer happens to export.
It scanned .env for keys from before the POSTGRES_* rename and refused to start when it found any. That is migration scaffolding for a userbase that does not exist — nobody is running this yet — and it broke the only person who is: the developer's own .env still carried DB_HOST and friends, so the server exited before connecting. The mechanism was wrong twice over. It was advisory noise when it landed, and the whole-branch review escalated it to fatal on the theory that a stale key could silently point the server at the wrong database. Both readings served a migration nobody has to perform. An unknown key in .env is simply ignored, which is what every other unset variable already does.
The cockpit now creates the whole identity chain — principals, keys, portfolios, accounts, broker connections and grants all have POST routes and pages behind them — so dev_identity.sql only duplicated that with a published secret and a hardcoded bcrypt hash. minimal_seed.sql was thin cover too: it inserts SPY with an Alpaca mapping, but with no credentials the connection cannot route, so it got you to "order accepted, routing fails" and no further. Real instruments come from `setup sync-broker`, which needs credentials anyway. Removing them makes an empty catalog the normal state after `database init`, so preflight's empty-instrument check can no longer be fatal — the operator needs the admin console up to enter broker credentials in the first place. Downgraded to a warning; every other preflight check is unchanged. Also drops the --fixtures flag, fixture_sql(), load_fixtures(), and the stale comments in auth.rs and brokers.rs that referenced the dev fixture.
…files The README's own quickstart did not work. `serve()` hard-exited when OMS_ADMIN_PASSWORD was unset — and admin auth defaults to on — so `cargo run` on a fresh clone died immediately after the one command that had just succeeded, contradicting the documented promise that .env is an override and not a prerequisite. OMS_BIND_ADDR had no default either. Both now default, gated by the rule config.rs already applies to role passwords: a shipped default is fine while the bind address is loopback and refused the moment it is not. bind_is_loopback() fails closed on anything it cannot parse, and deliberately treats 0.0.0.0 as exposed — it looks local and is not. The default-password guard itself was hand-copied between init and serve. It is now one helper the caller passes its own scope to: init creates both roles and checks both, serve connects only as oms_user and checks only that. The scopes differ on purpose — refusing to boot over mdm_master, which serve never uses, would block a working runtime config. Migration checks were one round-trip per file, so `migrate` and `status` on an up-to-date database spent 40-odd sequential queries confirming there was nothing to do. Now one SELECT per target and a set lookup. Deletes db/scripts/cleanup_orphaned_options.sql (never embedded, no command ran it, and its header told you to run it by hand with psql — the tooling the CLI exists to remove) and scripts/send_test_order.py with its private .env.example, superseded by clients/python and still referencing the long-gone book_id. .env.example was documenting about half of what the code reads; adds the missing broker, catalog-sync and feed keys. README documents boot-time auto-sync, which is now the normal way an empty catalog gets filled.
Replaces the mdm_master/oms_user split with a single role, `oms`, that owns the
database, both schemas and every table in them, and is also what the server
connects as. This is how NautilusTrader sets up its Postgres: `database init`
creates one role, grants it the schema outright, and the runtime connects as it.
The split was the last trace of the parked MDM design, where `public` was to be
owned by a separate service and the OMS would only read it. That service does not
exist, so the cost was all setup friction and no isolation: two passwords to set,
one of which authenticated nothing (mdm_master was reached only via SET ROLE from
the superuser connection, never logged into), and a role name nobody could decode.
Kept, deliberately unlike Nautilus:
- Versioned migrations. Nautilus re-runs CREATE TABLE IF NOT EXISTS files and
upgrades by dropping the database, which is safe because their Postgres is a
rebuildable cache. Ours holds orders, so `database migrate` stays.
- The public/oms schema split, now purely for consumers rather than permissions:
`public` holds master data another service can point at, `oms` holds this
application's operational tables. One role owns both.
MigrationTarget loses its `owner` field, which is now the same constant for every
target, and TARGETS is just the two schemas in dependency order.
Config drops MDM_MASTER_PASSWORD entirely; OMS_USER_PASSWORD becomes OMS_PASSWORD.
The `init` flag is --oms-password, kept distinct from --password so the role's
credential is never confused with the superuser's.
Existing databases need a one-off ownership reassignment; the commands are in the
PR description rather than an auto-migration, because dropping cluster-wide roles
is not something a tool should do unattended.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.