Conversation
urlparse().port is None when a DATABASE_URL omits the port, which rendered as the literal string "None" in ~/.pg_service.conf and ~/.my.cnf and broke every connection. Default to 5432/3306 in each parse_database_url.py so the generated config keeps its shape. Also stop swallowing the real psql error in the postgres wait_for_db retry loop -- it now surfaces the last stderr output once retries are exhausted instead of always reporting a generic timeout, so a misparsed URL diagnoses itself. Add regression coverage in both tests.sh scripts asserting PORT defaults correctly for a portless URL and that a live connection succeeds against the default port, since the existing DATABASE_URLs in docker-compose.test.yml always specified an explicit port and never exercised this path. Fixes #4
localstack/localstack:latest switched to a unified AWS image in March 2026 that requires a paid LOCALSTACK_AUTH_TOKEN to boot at all, even for plain S3, which breaks make test-postgres/test-mysql and CI with "Could not connect to the endpoint URL". Pin to 4.14.0, the last tag before that migration that still starts without a license. Revisit once the suite can target a supported, token-free community image.
blsmth
added a commit
to apppackio/apppack
that referenced
this pull request
Sep 2, 2026
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.
The generated MySQL client config only carried host/port/user, so a bare `mysql` (no --database, no positional dbname) had no default database -- unlike psql, which already gets dbname from ~/.pg_service.conf. This blocks moving `apppack db shell` off a hardcoded --database=<app name>, which breaks for externally-managed MySQL databases where the app name and the DATABASE_URL's database name diverge. Add database=$NAME, but scoped to the [mysql] section rather than [client]: mysqladmin and mysqldump also read [client] and reject `database` as an unknown option, so putting it there broke the health check and dump/load flow. load-from-s3.sh's own DROP/CREATE cycle also needed --no-defaults + explicit connection flags, since the app DB user has no privileges outside of $NAME and $NAME doesn't exist for the moment between the drop and the create. For managed AppPack databases $NAME already equals the app name (the same value the CLI passes via --database today), so this is a no-op for that path. Add a tests.sh assertion that a bare `mysql` connects to the database named in DATABASE_URL, matching the exact behavior the CLI will depend on.
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.
Summary
urlparse(...).portisNonefor aDATABASE_URLwith no explicit:port(e.g. Neon, Crunchy, Supabase, Render, PlanetScale connection strings). Bothpostgres/bin/parse_database_url.pyandmysql/bin/parse_database_url.pyprinted that as the literal stringPORT=None, which got written straight into~/.pg_service.conf/~/.my.cnf, making every client connection fail even though the port itself was reachable.parsed.port or 5432(postgres) andparsed.port or 3306(mysql). One-line change per file, config file shape is unchanged.postgres/bin/entrypoint.sh'swait_for_dbswallowedpsql's stderr, so a bad/misparsed URL just looked like a generic connectivity timeout after 30 retries. It now captures the last error and prints it once the retry budget is exhausted (loop stays quiet on intermediate attempts; retry count/sleep interval unchanged).postgres/tests/tests.shandmysql/tests/tests.sh: assertPORTresolves to5432/3306for a portlessDATABASE_URL, and assert a real connection succeeds against a portless URL. The existingdocker-compose.test.ymlfiles always used an explicit:5432/:3306, which is exactly why this was never caught.Closes #4.
Scope / no-op confirmation
Managed AppPack databases always include an explicit port in their
DATABASE_URL, soparsed.portis neverNonefor them andparsed.port or <default>is a strict no-op in that path. This change only changes behavior for externally-managed databases whose connection strings omit the port.Test plan
Ran both test suites locally with Docker (
make test-postgres,make test-mysql), both exit 0:Both GitHub Actions checks (
Test MySQL,Test Postgres) are green on this PR: https://github.com/apppackio/apppack-db-utils/actions/runs/33544838121, https://github.com/apppackio/apppack-db-utils/actions/runs/33544838115Unrelated pre-existing infra issue, fixed in this PR to unblock CI:
localstack/localstack:latestmoved to a unified AWS image in March 2026 that requires a paidLOCALSTACK_AUTH_TOKENto boot at all (confirmed reproducible onmainprior to this branch's changes, and the last green CI run onmainwas 2026-01-27).postgres/docker-compose.test.ymlandmysql/docker-compose.test.ymlnow pin thes3service tolocalstack/localstack:4.14.0, the last community-edition tag that starts without a license, with a comment explaining why and noting the pin should be revisited once a supported token-free image is available.Not run:
make image/make push-image(out of scope, publishing is a separate deploy step).