fix: pushkin-prep should gracefully handle containers already existing and non-Pushkin templates#372
Conversation
When running `pushkin-dev prep` with existing database containers from a previous run, the command would fail with a misleading "password authentication failed" error. This happened because the existing containers were using different credentials than the current pushkin.yaml config. This commit implements Option 1 from the issue: automatic detection and cleanup of stale database containers. Changes: - Added `ensureCleanState()` function that checks for existing database containers before setup - If containers exist, displays a warning and removes them cleanly - Uses `docker ps -a` to detect both running and stopped containers - Follows the same pattern as `killLocal()` using compose.stop() and compose.rm() - Includes proper error handling to avoid failing the entire setup Benefits: - Makes `prep` command idempotent - safe to run multiple times - Eliminates confusing password authentication errors - Improves developer experience by avoiding manual cleanup steps - No waiting through 10 failed connection attempts (~2+ minutes) Fixes pushkin-dev prep fails cryptically when database containers already exist
|
cherriechang
changed the title
cherriechang
changed the title
|
@cherriechang I've been hitting this issue while reviewing #373, so it would be nice to merge this ASAP! Is it ready for review? |
I think so! |
|
@becky-gilbert -- what's the status on this? |
|
@jkhartshorne sorry, I've been focused on #383 and #376. This fix did work for me when I hit the issue but I'd like to take a closer look at the changes before approving. Will do ASAP! |
becky-gilbert
left a comment
becky-gilbert
left a comment
There was a problem hiding this comment.
I happy this looks good and is working. @cherriechang do you want to test my changes before merging?
| await compose.down({ | ||
| cwd: dockerPath, | ||
| config: dockerConfig, | ||
| commandOptions: ["--volumes"], // removes named volumes (e.g. test_transaction_db_volume) |
There was a problem hiding this comment.
The -v flag on compose.rm only removes anonymous volumes, so I switched from compose.stop/compose.rm to compose.down in order to also remove named volumes: pushkin_test_db_volume, pushkin_test_transaction_db_volume. I ran into the same "password authentication failed" error with the original fix on this branch, which was because my test volumes survived the slate-cleaning process. The new container mounted the the old volume, which contained the PostgreSQL data directory with the old password.
The original fix did work for me before, but that was probably because either those named volumes hadn't been created yet or I'd cleared them.
Volume auth error before this change:
After:
| config: "docker-compose.dev.yml", | ||
| }); | ||
| } catch { | ||
| } catch (e) { |
There was a problem hiding this comment.
This bug wasn't introduced here, just cleaning it up anyway!
4 participants
1) Failing for existing container
When running
pushkin-dev prepwith existing database containers from a previous run, the command would fail with a misleading "password authentication failed" error. This happened because the existing containers were using different credentials than the current pushkin.yaml config.This commit implements Option 1 from the issue: automatic detection and cleanup of stale database containers.
Changes:
ensureCleanState()function that checks for existing database containers before setupdocker ps -ato detect both running and stopped containerskillLocal()using compose.stop() and compose.rm()Benefits:
prepcommand idempotent - safe to run multiple timesFixes pushkin-dev prep fails cryptically when database containers already exist
2) Failing for non-Pushkin site templates