Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setup database migration for staging and prod #601

Merged
merged 7 commits into from
Apr 17, 2023

Conversation

cychu42
Copy link
Contributor

@cychu42 cychu42 commented Apr 12, 2023

Part of #291

Explanation

Helpfully these explanations make it easier to understand what I'm doing and why, in this PR.

This PR should allow staging and production to use Prisma migration by default, through deploying migration files.
Migration should keep data in the database. Only when DATABASE_SETUP =1 is an environment variable, we would go back to force pushing and cleaning data like before.
For there to be anything to deploy, I made the first migration file too.

A script is setup to make creating migration files locally with docker easier. When Prisma creates a migration file, it requires permission to create a temporary shadow database to do this, which means the user of MySQL database needs CREATE, ALTER, DROP, and REFERENCES privilege. Therefore, I use root user to do this in the script, against the local docker database container. The script doesn't seed database or apply migration, just creating a migration file and also generating Prisma client (default behavior) in case a dev forgets.

Changes

  • Add migration in docker-entrypoint.sh, when DATABASE_SETUP !=1
  • Add migration file creation script, with required permission, in package.json
  • Create an initial migration file in prisma/migrations
  • Add documentation for making migration files in DEPLOY.md

Test

1.You can test the script npm run db:migration to create migration files using our local docker database container.

2.You can test deployment of the migration file via npx prisma migrate deploy, to an empty docker database container, or one that already has this migration applied to. It should either work or tell you no migration is needed.
Note: this command doesn't check for database drift.

Having this run on staging would be the real test.

* Add migration in docker-entrypoint.sh, when DATABASE_SETUP !=1
* Add migration file creation script, with required permission
* Create initial migration file
* Add documentation for making migration files
@cychu42 cychu42 added category: deployment Related to building our local code into a working unit category: data Anything related to data management and structure labels Apr 12, 2023
@cychu42 cychu42 added this to the Milestone 1.0 milestone Apr 12, 2023
@cychu42 cychu42 requested review from humphd and sfrunza13 April 12, 2023 20:50
@cychu42 cychu42 self-assigned this Apr 12, 2023
@cychu42 cychu42 modified the milestones: Milestone 1.0, Milestone 1.0a Apr 12, 2023
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a few comments, but I think this looks great.

I have one question: the Prisma docs talk about running this via CI. We're currently doing it differently, by running it on startup for the container. This means both instances will run it. I think this means we should adjust our Docker deploy settings to only bring up 1 instance at a time vs. doing both at once. We also need to shut down the existing ones first (i.e., we can't have zero-downtime deploys, which is OK). My reading of the docs indicates that the second instance will skip doing most of the work, since the db will already be in sync (i.e., it should be safe to do this in each instance). Do you agree?

docker-entrypoint.sh Outdated Show resolved Hide resolved
docker-entrypoint.sh Show resolved Hide resolved
@humphd
Copy link
Contributor

humphd commented Apr 12, 2023

For the docker changes, see https://docs.docker.com/compose/compose-file/deploy/#update_config. Specifically, I think you need to update our docker compose files:

parallelism: 1
delay: 20s
failure_action: rollback
order: stop-first

This is my reading of it, but confirm what you think. We'll have to play with this on staging to know for sure.

@humphd humphd requested a review from a user April 12, 2023 21:19
* Adjust docker deployment for update
* Add comments to database_migration and database_setup
@cychu42
Copy link
Contributor Author

cychu42 commented Apr 13, 2023

This is my reading of it, but confirm what you think. We'll have to play with this on staging to know for sure.

I agree with what you said.
While Prisma does have advisory locking, which would skip repeated deploy within last 10 seconds (can't be configured), it's probably safer to do what you suggests.

I detected an issue with using our old npx prisma db push --force-reset command to setup database for the first time.
It would likely cause conflict with our initial migration file. For example, we might already pushed a user table with this command but that migration file wants to create user table too.
Instead, I'm going to use npx prisma migrate reset --force --skip-seed, which resets/wipes the database and applies migration files, and it would know what migration is applied already and skip them, instead of causing conflict.
--force skips confirmation, and --skip-seed skips seeding.

I also added a bit more explanation in DEPLY.md for what a migration file is.

@cychu42
Copy link
Contributor Author

cychu42 commented Apr 13, 2023

Thinking about it, I added extra warning in DEPLOY.md about schema change without Prisma migration.
Also expanded comments for database_setup vs database_migration as suggested.

* Add more explanation for database_setup vs database_migration
@cychu42 cychu42 requested a review from humphd April 13, 2023 17:03
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good. Made a few corrections/suggestions.

I'd also like to get @dadolhay's blessing on this approach.

I'm excited to try this on staging. Before we land it, let's co-ordinate so that I can change the service on staging (it currently runs DATABASE_SETUP=1 always).

DEPLOY.md Outdated Show resolved Hide resolved
DEPLOY.md Outdated Show resolved Hide resolved
DEPLOY.md Outdated Show resolved Hide resolved
DEPLOY.md Outdated Show resolved Hide resolved
docker-entrypoint.sh Outdated Show resolved Hide resolved
docker-entrypoint.sh Outdated Show resolved Hide resolved
docker-entrypoint.sh Outdated Show resolved Hide resolved
docker-entrypoint.sh Outdated Show resolved Hide resolved
@cychu42
Copy link
Contributor Author

cychu42 commented Apr 13, 2023

OK. Thank you for catching those and making suggestions. I made the changes and added a bit along the way, like stating the database in docker needs to run locally for npm run db:migration to work.

DEPLOY.md Outdated Show resolved Hide resolved
docker-production.yml Outdated Show resolved Hide resolved
humphd
humphd previously approved these changes Apr 13, 2023
Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome.

Just remember, ping me before you are going to merge this.

@cychu42
Copy link
Contributor Author

cychu42 commented Apr 13, 2023

We would want to use DATABASE_SETUP=1 for the first time running this PR on staging and production, since no migration has been done on them yet and Prisma would just be confused why all those tables already exist.

If nothing goes wrong from there, we can switch it off for future updates.

@ghost
Copy link

ghost commented Apr 14, 2023

@cychu42 @humphd
I'd suggest using something more dramatic, like: DANGER_DATABASE_WIPE_REINITIALIZE

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see comment above

@cychu42
Copy link
Contributor Author

cychu42 commented Apr 14, 2023

I changed all mentions of DATABASE_SETUP to your spooky version, including the same name function and variable.

@cychu42 cychu42 requested review from humphd and a user April 14, 2023 22:01
# See if we need to do database wipe and reinitialize before starting.
if [[ $DANGER_DATABASE_WIPE_REINITIALIZE == "1" ]]; then
# Run database wipe and reinitialize
DANGER_DATABASE_WIPE_REINITIALIZE
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the fn call on line 10, should this not be lower case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right.

@cychu42 cychu42 requested a review from a user April 15, 2023 14:18
@humphd
Copy link
Contributor

humphd commented Apr 16, 2023

@dadolhay can you review this again please?

@humphd humphd modified the milestones: Milestone 1.0a, Milestone 1.0 Apr 16, 2023
@cychu42 cychu42 merged commit 13f9b62 into DevelopingSpace:main Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category: data Anything related to data management and structure category: deployment Related to building our local code into a working unit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants