fix(cli)!: ensure env vars are set before migrating #1222
Conversation
|
Should this be seen as a breaking change? 🤔 |
|
Kind of yes, because it would prevent the migration from running w/o specifying a URL or env vars. The previous check is ignored since |
Okay, I will think about it Would it be okay for you if this fix wont immediately merged because of that? |
Shinigami92
added
the
breaking change
Shinigami92
changed the title
Shinigami92
changed the title
|
Alright, I'll also take a look at some other open issues |
Shinigami92
added
the
do NOT merge yet
| if (!cp.host && !cp.port && !cp.database) { | ||
| if ( | ||
| !process.env[argv[databaseUrlVarArg]] && | ||
| (!process.env.PGHOST || !cp.user || !cp.database) |
There was a problem hiding this comment.
question (if-minor): you removed the check for !cp.host && !cp.port, but added !cp.user and used || (or). Can you explain a bit why you did it like that?
There was a problem hiding this comment.
cp.host and cp.port are unlikely to be undefined since we read pg defaults, which set host to 'localhost' and port to '5432' if missing.
When running pg locally without any env vars, node-pg-migrate would run but give an error due to an undefined password string/function. Adding a check for the pw alone isn't enough because it would still cause a connection error if pg isn't running locally (also when pw is set but others vars aren't).
so, if there's no dburl, I check if any required variable or process.env.PGHOST is missing and stop it
fixes #1220
pglibrary defaultshostto 'localhost' whenPGHOSTenvironment variable is missing, potentially leading to unintended database connections, To mitigate this, we now perform an explicit check to ensure thePGHOSTenvironment variable is set.Default Config
Reference: