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

[WIP] added experimental multidb support #256

Closed
wants to merge 1 commit into from

Conversation

tg44
Copy link
Contributor

@tg44 tg44 commented Dec 15, 2019

It's probably not a single-commit change, but I will summarize what I done;

  • I made the Dockerfile to work as a docker file (the build is stateless)
    • this should be either reverted or copied over to the other archtypes
    • also added a dockerignore
    • it was a "must" bcs I can't make my mac to build this project correctly
    • probably a "how to contribute" readme would solve this also
  • updated the knex dep
    • also went to mysql2
    • also added the sqlite3
    • also added the pg
  • added a knex-native engine (db.js and install.md)
  • fixed the migration (20190227065017_settings.js)
    • as I understand we start a db and migrate it, and while we in a transaction to migrate we also try to write the db from an another (or maybe from the same?) db connection. This is simply not working with sqlite bcs you have only one thread/connection/whatever to write the db. Also it seems to be a bad thing according the knox github issues.
  • moved the insert statement from the migration to the setup (setup.js)
    • also refactored the file a bit so its probably more readable
  • added a now_helper
    • bcs sqlite is a dummy
  • used the now_helper instead of the Model.raw

If you want atomic commits I think the correct steps would be:

  • write a developer doc OR fix docker
  • factor out now_helper
  • factor out + fix the migration and refactor the setup
  • add the new engine + deps + extend the now_helper

It is working with sqlite3 right now. I didn't test mysql, and pg. (Also probably would be nice to make the compose stack to support examples to all three and add default configs to all three.)

(I love the idea and the functionality BTW, really nice app!)

Probably fixes #185 and fixes #200

@jc21
Copy link
Member

jc21 commented Feb 19, 2020

If you're still considering doing this PR, please open again against the develop branch.

@jc21 jc21 closed this Feb 19, 2020
@tg44
Copy link
Contributor Author

tg44 commented Feb 19, 2020

Maybe I'm the sensitive, but this is kinda rude. I made this PR ~3 month ago. You don't even comment it, and now I should reopen it?

Ofc I will do it bcs I want to work with sqlite instead of mysql, but still, at least you could mention if its a good or a bad set of modifications...

@jc21
Copy link
Member

jc21 commented Feb 19, 2020

Honestly, I haven't had time to invest in this project over the last 6 months. I'm happy for this change to go in once it's fully tested, but since the CI was down at the time you submitted the PR, there hasn't been a docker image for anyone to test with.

@jc21
Copy link
Member

jc21 commented Feb 19, 2020

CI is back though, your next pr will be built for you and others to test.

@LouisSung
Copy link

Hi, @jc21 and @tg44
Thank you two for making this project and sending PR
I still have no time on trying this project, but I'm surveying a GUI-based solution for Nginx.
However, run an external database might cost performance, that's why I wonder if SQLite is supported and this PR is still available.
Or there's any reason to use a standalone database rather than SQLite? Since I don't know if there are any constrains or just the preferences?

Thank you~

@tg44
Copy link
Contributor Author

tg44 commented Jun 12, 2020

I think it can be recreated based on this PR. I run a mysql server right now (for basically no reason :( ). If you need further help, I try to help, just summon me to some comment section.

@tg44 tg44 mentioned this pull request Jul 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updating the database client No-config databases or json data storage
3 participants