Multidb - sqlite support #510
Conversation
|
I think it is a CI error and not an error with the code... |
|
|
||
| module.exports = function () { | ||
| return new Promise((resolve, reject) => { | ||
| return setupJwt(resolve, reject); |
There was a problem hiding this comment.
setupJwt is not a promise, so returning it as is will be async but won't reject on error properly. If it were a promise, you wouldn't need to wrap it in a Promise constructor either.
There was a problem hiding this comment.
I literally just extracted the code to a new function. I think if it has some logical error now, it had it in the past too.
Probably I could have moved the promise creation to the setupJwt too, but probably it is just a matter of taste.
There was a problem hiding this comment.
ohhh, or I just simply not need to write the "return" keyword in this case?
There was a problem hiding this comment.
Not really, it just could be written differently but let's just get it through for now
|
|
||
| return userModel | ||
| function setupDefaultSettings() { |
There was a problem hiding this comment.
Not quite sure why you took this from a Migration and put it here, you're not doing that to any other migrations...
There was a problem hiding this comment.
This is the only migration where the concept was broken. I didn't needed to do it in any other migration bcs all of the other migrations just modify the schemas, and not trying to insert things to it. (BTW; we insert users in the setup but we insert settings in a migration?)
As I understand when you run the migrations it connects to the db. And when you run the settingModel.query().insert() it tries to connect to the db again (while we still migrate). Which in multithreaded env (like pg or mysql) not a big deal, you can have multiple connections/connection pools to the same db. But in sqlite you hava a lock on the file, so while one connection is attached to the db, the other connection "can't" connect to it.
If you are interested in it deeper I referenced an issue in the commit message.
There was a problem hiding this comment.
Ah ok I get your point about breaking the concept. All good.
To clarify, "migrations" should be changes that occur before the App really runs. It's used on the Knex layer and should not perform any logic on data and assumes the database is in a certain state prior to execution.
Setup users is part of the app because it has to check if there is an admin user and only when not found, it creates one. As this happens on startup each time, a migration is not the right place for that.
There was a problem hiding this comment.
I understand this, BUT if injecting users is a runtime check, inserting settings why not? Also if inserting settings is essential to start the app, I think the default user is essential to start the app too. It breaks the logic if one or the other handled differently. I think it's mostly a theoretical question if inserting default data is part of the db migration or not, and most of the JVM world would place them to the migrations (including myself), but here the code already introduced both ways, and I simply chose the comfy side.
|
Docker Image for build 4 is available on DockerHub as |
|
Starting from today I use this image on my home server. |
|
Thanks the merge! |
I recreated this PR; #256
Separated commits;
transaction in transactionconnection in connection problem in the migration, which causes problems on db-engines which has no multithread support (like sqlite3), also adds a minor refactor to the setup.jsknex-native) and let an experienced knex user to add some own knex-native configurations. Also, this commit showcases an sqlite3 config, and adds psql and sqlite3 libs.Probably fixes #185 and fixes #200
Missing things;