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

Added support for a database installer script. (#438) #676

Merged
merged 1 commit into from
Apr 13, 2020
Merged

Added support for a database installer script. (#438) #676

merged 1 commit into from
Apr 13, 2020

Conversation

jeremyphilemon
Copy link
Contributor

Link the Issue(s) this Pull Request is related to.
Fixes #438

Summarize your change.

  • Added a bash script in sandbox/ called setup-database.sh – automates the installation and setting up of the database, including applying migrations.
  • The steps for installation were taken from the "Getting Started" Guide
  • Uses the quick fix referred to in Latest Postgres docker needs additional environment variables #651 instead of the one in the documentation due to a breaking change in docker-library's postgres image.

It's also helpful to describe why you're making this change.

When installing and setting up the database, there are numerous steps that can go wrong. It would be nice to have an installer script to guide you through the database install and configuration. This same script could also be used to update and apply migrations to existing databases. - @gregdenton

Hence :)

Testing

  • From the root directory of the codebase run ./sandbox/setup-database.sh

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Mar 31, 2020

CLA Check
The committers are authorized under a signed CLA.

Copy link
Collaborator

@bcipriano bcipriano left a 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.

@jeremyphilemon
Copy link
Contributor Author

@bcipriano sounds good! I'll make the changes and update the PR!

@jeremyphilemon
Copy link
Contributor Author

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 :)

@bcipriano
Copy link
Collaborator

Sorry for the delay here.

That's docker-compose.yml you're talking about, right?

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.

@gregdenton
Copy link
Collaborator

I agree with Brian, lets keep the docker-compose.yml but we can certainly update the file to utilize the script and make the deployment patterns more similar.

Copy link
Collaborator

@gregdenton gregdenton left a 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.

@jeremyphilemon
Copy link
Contributor Author

@bcipriano @gregdenton Alright, sounds good! :)

@bcipriano bcipriano merged commit 85f30de into AcademySoftwareFoundation:master Apr 13, 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.

Database installer script
3 participants
  翻译: