Skip to content

Enable db commands for externally-managed databases - #173

Open
blsmth wants to merge 8 commits into
mainfrom
issue-146-external-database
Open

Enable db commands for externally-managed databases#173
blsmth wants to merge 8 commits into
mainfrom
issue-146-external-database

Conversation

@blsmth

@blsmth blsmth commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Closes #146.

Adds --external-database <postgres|mysql> to apppack create app, which sets a new ExternalDatabaseEngine CloudFormation parameter so db shell/db dump/db load can target an externally-managed database (Neon, Crunchy, etc.) reachable via the app's DATABASE_URL config variable. Also adds a matching interactive prompt in AskForDatabase (for apppack modify app), client-side validation (mutually exclusive with --addon-database/--addon-database-name, engine must be postgres or mysql), and replaces the unhelpful "unknown database engine " error with a targeted message pointing users at either apppack modify app/--external-database or apppack create database.

Depends on the formations change (apppack-backend) landing and being released first — until formations emits dbutils.engine for externally-configured apps, setting this flag has no effect.

Caveat carried over from the spec: apppack db load's load-from-s3.sh script likely does a DROP/CREATE of the target database, which some managed providers (e.g. Neon) may not permit for the app's role. db shell and db dump are the confident wins here; db load may not reach full parity even after this lands.

Happy path (DBShellTaskInfo/DBDumpLocation/DBDumpLoadFamily, isPostgres in cmd/db.go) is unchanged.

Want a Test Plan section added, or is this concise enough as-is?

@ipmb

ipmb commented Aug 12, 2026 via email

Copy link
Copy Markdown
Member

blsmth added a commit that referenced this pull request Aug 19, 2026
@blsmth
Infer external database engine from DATABASE_URL, drop the flag
79c368f 
ipmb reviewed #173 and pointed out the user should never have to type
the database engine -- it's inferable from the DATABASE_URL scheme.
Delete --external-database and both interactive prompts; SetInternalFields
now auto-detects the engine from the app's DATABASE_URL config variable
via SSM, for both `create app` and `modify app`.

A managed AppPack database and pipelines/review apps always force the
field back to "", so this is idempotent no matter how many times
`modify app` runs. Any SSM/parse failure is silent (the common case,
since `create app` runs before the app exists); an unrecognized scheme
still gets one warning line, since there's no longer a flag to fall
back on.

The formations-side CloudFormation parameter and condition are unchanged
-- every db-utils resource is still gated on DatabaseEnabled, and there
is no task definition to run for an external DB without it.
@blsmth

blsmth commented Aug 19, 2026

Copy link
Copy Markdown
Contributor Author

Pushed an update that addresses your review: --external-database and both interactive prompts (enable? / which engine?) are gone. SetInternalFields now infers ExternalDatabaseEngine from the app's DATABASE_URL config variable (read from SSM), comparing only the URL scheme -- postgres/postgresql/pgsql/psql -> postgres, mysql/mysql2/mariadb -> mysql. This runs for both create app and modify app, so the resulting workflow is:

apppack create app ...
apppack config set DATABASE_URL=...
apppack modify app <name>   # picks up the engine automatically

A managed AppPack database (--addon-database/--addon-database-name) always wins and forces the field back to "", and pipelines/review apps are excluded, since the CloudFormation condition requires IsApp -- so detection is idempotent no matter how many times modify app runs.

On the "no CloudFormation changes" half of your comment: that part isn't achievable. Every db-utils resource (task definitions, execution/task roles, log group, S3 bucket) is gated behind DatabaseEnabled, and app.StartTask only ever does DescribeTaskDefinition -> RunTask -- there's no task-definition family to run against for an external DB without the formations change. That half of the PR is unchanged.

One tradeoff worth flagging: since there's no flag anymore, there's also no escape hatch for a DATABASE_URL with a scheme we don't recognize -- we print one warning and leave db shell/db dump disabled rather than failing the deploy. Happy to add --external-database back as a hidden override for that case if you'd rather have one. Let me know.

@ipmb

ipmb commented Aug 19, 2026 via email

Copy link
Copy Markdown
Member

blsmth added 2 commits August 24, 2026 12:07
@blsmth
Add --external-database flag for externally-managed databases
2c30a20 
Registers ExternalDatabaseEngine as a CloudFormation parameter on
apppack create app (postgres|mysql), validates it is mutually
exclusive with --addon-database/--addon-database-name, and adds a
matching interactive prompt in AskForDatabase for apppack modify app.

Also replaces the unhelpful "unknown database engine " error (empty
engine) with a targeted message that checks for a DATABASE_URL config
variable and tells the user how to enable db utils or create a
database.
@blsmth
Infer external database engine from DATABASE_URL, drop the flag
b31a056 
ipmb reviewed #173 and pointed out the user should never have to type
the database engine -- it's inferable from the DATABASE_URL scheme.
Delete --external-database and both interactive prompts; SetInternalFields
now auto-detects the engine from the app's DATABASE_URL config variable
via SSM, for both `create app` and `modify app`.

A managed AppPack database and pipelines/review apps always force the
field back to "", so this is idempotent no matter how many times
`modify app` runs. Any SSM/parse failure is silent (the common case,
since `create app` runs before the app exists); an unrecognized scheme
still gets one warning line, since there's no longer a flag to fall
back on.

The formations-side CloudFormation parameter and condition are unchanged
-- every db-utils resource is still gated on DatabaseEnabled, and there
is no task definition to run for an external DB without it.
@blsmth
blsmth force-pushed the issue-146-external-database branch from 79c368f to b31a056 Compare August 24, 2026 16:07
blsmth added 2 commits August 24, 2026 12:16
@blsmth
Stop advertising the removed --external-database flag
ca32cf3 
The engine is now inferred from DATABASE_URL, so the "no database
configured" error was pointing users at a flag that no longer exists --
following its advice would fail with "unknown flag". Drop the
parenthetical and let `apppack modify app` be the single answer.

Turn the test that asserted on the flag name into a negative assertion,
so a stale reference can't creep back into user-facing text.
@blsmth
Restore files dropped by a mis-staged commit
d2b81c4 
The previous commit was made against an index that already held staged
deletions from a concurrent rebase in this worktree, so it removed the
go vet/fmt CI gate, the flag-shorthand collision tests, and unrelated
CHANGELOG and config entries. Restore all five from main; none of them
have anything to do with this branch.
@blsmth blsmth changed the title Add --external-database flag for externally-managed databases Enable db commands for externally-managed databases Aug 24, 2026
blsmth added 2 commits August 26, 2026 09:44
@blsmth
Point users at modify app after DATABASE_URL is set
a110907 
Setting DATABASE_URL is the obvious move when pointing an app at an
externally-managed database, but on its own it does nothing for
`apppack db shell`/`db dump`/`db load` -- those need the db-utils
resources, which only appear when the stack is updated. Nothing told the
user that second step existed, so the discoverable path dead-ended at a
config variable that looked sufficient.

Print a follow-up hint from `config set` and `config import` when
DATABASE_URL lands on an app whose db utils aren't enabled yet. Stays
quiet when an engine is already set (managed database, or a previous
`modify app`), and for pipelines/review apps, where external databases
are gated off in CloudFormation anyway.

Advisory only: the variable is already stored by the time we're called,
so a settings-load failure just skips the hint rather than failing a
command that succeeded.

Suggested by ipmb in review of #173.
@blsmth
Fix DeepSource findings on PR #173
aed9634 
- Add `tt := tt` loop-variable copies in app/db_test.go and
  cmd/config_test.go table tests. These are false-positive VET-V0010
  hits: go.mod requires go 1.25.4, where each `for` iteration already
  gets its own variable, but DeepSource's loopclosure analyzer still
  assumes pre-1.22 semantics whenever it sees t.Parallel() inside a
  t.Run closure. Matches the existing skipcq-free remediation used
  elsewhere in the repo.
- Suppress CRT-P0003 on detectExternalDatabaseEngine and the
  fetchDatabaseURL function variable in stacks/app_pipeline.go via a
  trailing `// skipcq: CRT-P0003` comment, following the precedent in
  app/utils.go. Passing aws.Config by value is the SDK convention used
  throughout this codebase, so the signature stays unchanged.
- Drop the bogus t.Helper() calls from TestEngineFromDatabaseURL,
  TestDetectExternalDatabaseEngine, and
  TestDetectExternalDatabaseEngineIdempotent -- t.Helper() has no
  effect on a top-level Test function and was left over from a copy
  of the table-test template. TestEngineFromDatabaseURL is a pure
  function test, so it gets t.Parallel() like its siblings. The other
  two stub the package-level fetchDatabaseURL variable and must not
  run in parallel with each other, so they stay serial with no
  t.Parallel() call (their subtests were never parallel either).
@blsmth blsmth mentioned this pull request Sep 1, 2026
Open
@blsmth
Parallelize TestEngineFromDatabaseURL subtests
6a65172 
Adding t.Parallel() to the parent test in aed9634 tripped GO-W6007:
DeepSource requires subtests to call t.Parallel() too once the parent
does. TestEngineFromDatabaseURL's subtests only exercise the pure
engineFromDatabaseURL function, so this is safe. Add the standard
tt := tt loop-variable copy alongside it so the added closure over
tt/t.Parallel() doesn't re-trigger the VET-V0010 loopclosure false
positive on this file.

TestDetectExternalDatabaseEngine and TestDetectExternalDatabaseEngineIdempotent
stay serial since they mutate the package-level fetchDatabaseURL stub.
@blsmth
Fix db shell for externally-managed MySQL databases
b3e9afe 
DBShellTaskInfo() built `mysql --database=<app-name>`, which only works
for a managed AppPack database (named after the app). An externally-
managed MySQL (e.g. PlanetScale) has whatever database name is in its
DATABASE_URL, so the connection failed.

For non-review apps, use a bare `mysql` and let the db-utils image
supply the database, the same way psql already does via
~/.pg_service.conf. Review-app behavior is untouched: review apps
cannot use external databases (the CloudFormation condition requires
IsApp), so they keep the explicit `--database=<app>-pr<N>` form
byte-for-byte.

db dump and db load are unaffected -- dump-to-s3.sh/load-from-s3.sh
already derive the database name from DATABASE_URL via $NAME.

Deploy ordering: this changes the managed-database command too (bare
`mysql` instead of `mysql --database=<app>`), which only works once
the db-utils image writes `database=$NAME` into ~/.my.cnf (in flight
as apppackio/apppack-db-utils#5). This is safe because the image is
served from a mutable tag
(public.ecr.aws/d9q4v8a4/apppack-db-utils:mysql, rebuilt from main)
with no version pinning, and db-utils ships ahead of the CLI -- so
there's no window where the CLI's new bare `mysql` command reaches a
db-utils image that doesn't yet resolve the database from
DATABASE_URL.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow DB commands against external databases

2 participants

@blsmth @ipmb