-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
Revamped docker compose #3324
Revamped docker compose #3324
Conversation
…cy of DJANGO_DATABASE_URL from dev
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.
Overall, this is really great. Nice work, @Anish9901 !
I made some changes to get the unified docker image to work. I'd like for you to look at those before we have others work through this.
x-config: &config | ||
# (Required) Replace '?' with '-' followed by your 50 character long Django | ||
# SECRET_KEY, you can generate one here: https://djecrety.ir/ | ||
SECRET_KEY: ${SECRET_KEY:?} |
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.
I'm debating whether we should include a default key that's obviously unfit for production. Something like:
SECRET_KEY: ${SECRET_KEY:?} | |
SECRET_KEY: ${SECRET_KEY:-PLEASEDONOTUSETHISSECRETKEYITISSUPERDUPERINSECURE} |
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.
After some discussion in various channels, I think the current failure state is better in this use-case.
@kgodey For testing, you need to run
from the root of the repo. This is because we haven't yet published the new images for 0.1.4. Actual users won't need to do that. Then, just copy the docker compose file to any directory you want to use for the test installation, and follow the instructions in that file. I recommend using some directory other than the repo to avoid contamination from any dev files (e.g., |
export POSTGRES_PASSWORD=mathesar | ||
export POSTGRES_HOST=localhost | ||
export POSTGRES_PORT=5432 | ||
export POSTGRES_DB=mathesar_django | ||
if [[ -z "${MATHESAR_DATABASES}" ]]; then |
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.
@mathemancer this script fails when all the POSTGRES_*
variables are explicitly provided but MATHESAR_DATABASES
isn't.
Here is the command I ran:
docker run \
-e POSTGRES_DB=mathesar_django \
-e POSTGRES_HOST=localhost \
-e POSTGRES_PORT=5432 \
-e POSTGRES_USER=mathesar \
-e POSTGRES_PASSWORD='mathesar' \
-v static:/code/static \
-v media:/code/media \
-v postgresql_config:/etc/postgresql/ \
-v postgresql_data:/var/lib/postgresql/ \
--name mathesar_service \
-p 8000:8000 \
--restart unless-stopped \
mathesar/mathesar-prod:latest
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.
Good catch; I've pushed a commit to fix it.
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.
Works perfectly after the fix, Brent!
I've unassigned @mathemancer and assigned @kgodey per this comment from @mathemancer |
After chatting in other channels, I'm going to merge this, and we'll handle issues in future PRs. |
Fixes #3306
For the reviewer:
docker-compose.yml
file, read through the comments and try setting up the prod version of Mathesar.Checklist
Update index.md
).develop
branch of the repositoryvisible errors.
Developer Certificate of Origin
Developer Certificate of Origin