Feature/add wait status check to pg upgrade #1488
Conversation
packages/pg-v5/commands/promote.js
Outdated
|
|
||
| if (status['waiting?']) { | ||
| throw new Error(`Database cannot be promoted while in state: ${status['message']} | ||
| \nIf you would line to promote this database while it is in state: ${status['message']}, please rerun with '--force'.`) |
There was a problem hiding this comment.
I wonder if we should be more... forceful... that we don't recommend this and it can/will cause issues if they force.
There was a problem hiding this comment.
I agree with @calebthompson. We could also include something in our message about using pg:wait (since right now this error message doesn't tell them what they should be doing instead).
I also wonder if we could import the waitFor helper from pg:wait and have this command wait for the dbs to be available as well. We could also add a --skip-wait-for-ready flag to use in combination with a --confirm flag to make it less convenient to do the wrong thing.
e.g. the workflow i'm proposing is this:
heroku pg:promote # waits for db to be ready, shows loading spinner to user
heroku pg:promote --skip-wait-for-ready # exits with error telling user to either use pg:promote, which will wait, or use --skip-wait-for-ready --confirm app
heroku pg:promote --skip-wait-for-ready --confirm myappname # display warning but promote anywaythere are some examples of confirming the app name throughout the codebase, here's one:
What do you all think about that workflow?
There was a problem hiding this comment.
Thanks for the 👀 all,
+1 on making the messaging more clear about potential consequences.
@fivetanley I think my preference would be to not perform the same extended waiting behavior as pg:wait in this command. Reasoning for this is
- users should already accustomed to running
pg:waitpost-upgrade and pre-promotion due to it being our documented method of upgrading a production database. - this is the last critical step before a new database is promoted to production. I think we should fail early and loud and not risk having a
waithold up the process or silently fail like it does in cases that prompted this PR. - the goal of this PR is to put some additional safety in place, and not alter the workflow of database promotion.
What if we cleaned up the messaging and left the command structure as is?
Error: Database in state \'${not-ready-state}\'.
Promoting this database will lead to a application errors and outage. Please run `pg:wait` to wait for database to become available.
To ignore this error, you can pass the --force flag to promote the database and risk application issues.
Or something to that effect
This PR adds safety bumpers to prevent users from promoting a database before it is in an available state.
This is specifically targeted at users who rely on
pg:waitto tell them when a database is in anavailablestate. The issue is thatpg:waitcan be terminated on the client side giving the impression that that the database is available. This PR adds checks to prevent that from happening.