search

knightcrawler-stremio / knightcrawler

A selfhosted Stremio addon
Apache License 2.0
265 stars 38 forks source link

Remove alter param from database sync #68

Closed FunkeCoder23 closed 8 months ago

FunkeCoder23 commented 8 months ago

Re: sequelize docs Alter is a bad idea in production.

Also print error message instead of sequelize error object as it's a bit verbose

sleeyax commented 8 months ago

The alter option is useful during development though, so perhaps we should make it configurable and only enable it in development mode? Ideally we'd have database migrations but I don't think those exist yet.

purple-emily commented 8 months ago

What’s the alter option do?

sleeyax commented 8 months ago

What’s the alter option do?

It's explained further up in the sequalize docs:

User.sync({ alter: true }) - This checks what is the current state of the table in the database (which columns it has, what are their data types, etc), and then performs the necessary changes in the table to make it match the model.

It pretty much auto-migrates the table to match the model definition, which is probably fine during development as you can just rebuild your datbase in case it messed something up. But that's obviously a bad thing to do in production.

purple-emily commented 8 months ago

Requesting a review from anyone. Code still compiles for me, I don't know if it totally solves #66 but from the comments in the issue, it seems that way.