-
Notifications
You must be signed in to change notification settings - Fork 198
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
Added support for a database installer script. (#438) #676
Added support for a database installer script. (#438) #676
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one change to request -- let's add a note/disclaimer to the top of the script explaining that this is experimental and isn't recommended for general use yet.
It's basically a translation of the existing process and that's a great start. As a followup change we'll want to look at integrating this into the rest of the sandbox process and see what changes that entails.
Once the script is proven in that scenario, we can start looking into updating the docs to see how this script can replace the existing manual process.
@bcipriano sounds good! I'll make the changes and update the PR! |
* Added disclaimer.
Updated! So here's my understanding: the sandbox currently uses docker.yml to set up the database. If we're looking to integrate this script into the rest of the sandbox process, which like you mentioned earlier is a translation of the existing process, will we be dropping the usage of docker.yml? I'm under the impression that sandbox is used to build up a quick configuration using docker and deploy the database, cuebot, and rqd with minimal configuration. Hence the reason to why it doesn't fall in line with the existing process. But isn't the existing process for production? Would like to have that clarified :) |
Sorry for the delay here. That's My rough idea was not to drop using that file entirely, but just update the db/flyway sections of it to use this new script instead. Anyway I don't think it's necessary in this PR -- this is a good start and we can figure out next steps over on the issue. |
I agree with Brian, lets keep the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the update!
Changes LGTM.
@bcipriano @gregdenton Alright, sounds good! :) |
Link the Issue(s) this Pull Request is related to.
Fixes #438
Summarize your change.
setup-database.sh
– automates the installation and setting up of the database, including applying migrations.It's also helpful to describe why you're making this change.
Hence :)
Testing
./sandbox/setup-database.sh