-
Notifications
You must be signed in to change notification settings - Fork 54
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
Update Docker setup and instructions #4351
Conversation
The Python REPL wasn't working because its environment variables were being overridden instead of extended. Also, due to the changes with how we mount everything, the web container no longer had the python startup file mounted. This fixes both those issues by moving the file and env variable to the base << *uber config. Also updates the instructions based on an issue I ran into with setting up some of our events.
@@ -31,6 +32,7 @@ x-dev-volumes: | |||
- &plugin-config-file-1 MAGSTOCK_CONFIG_FILES=config.ini | |||
- &plugin-config-file-2 MAGWEST_CONFIG_FILES=config.ini | |||
- &plugin-config-file-3 MAGPRIME_CONFIG_FILES=config.ini |
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.
Is there a reason to rename everything to config.ini rather than use:
- &plugin-config-file-3 MAGPRIME_CONFIG_FILES=config.ini | |
- &plugin-config-file-1 MAGSTOCK_CONFIG_FILES=magstock.ini | |
- &plugin-config-file-2 MAGWEST_CONFIG_FILES=magwest.ini | |
- &plugin-config-file-3 MAGPRIME_CONFIG_FILES=magprime.ini |
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.
Hm, good point. That's to mirror the one-event setup, which uses config.ini as the filename so you only have to edit the name of the _CONFIG_FILES env var. We can take it out of this setup though.
@@ -45,6 +47,7 @@ services: | |||
- *plugin-config-file-1 | |||
- *plugin-config-file-2 | |||
- *plugin-config-file-3 | |||
- *plugin-list |
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.
It might be easier at this point to just move the reference up one level and copy the whole environment section to every uber container.
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 don't expect most people to have to mess with the EVENTNAME compose project name, though, so it seemed like overkill to make everyone deal with it.
One thing I have considered is using an .env file instead of defining the config in the docker compose. That might be worth doing now and it might be even better in the future.
docker-compose.yml
Outdated
- uber_dev_box=True | ||
volumes: | ||
- $PWD:/app/ | ||
- $PWD/../magprime:/app/plugins/magprime | ||
- $PWD/.pythonstartup.py:/app/.pythonstartup.py |
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.
This should be available already as part of - $PWD:/app/
since it mounts in the whole directory.
/app # ls -la .*.py
-rw-r--r-- 1 root root 1226 May 7 01:25 .pythonstartup.py
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.
Huh... I swear my web container didn't have it, but now it does. Not sure how that happened, but I'll remove this line.
I may have gone a little overboard on the docs
The Python REPL wasn't working because its environment variables were being overridden instead of extended. Also, due to the changes with how we mount everything, the web container no longer had the python startup file mounted. This fixes both those issues by moving the file and env variable to the base << *uber config. Also updates the instructions based on an issue I ran into with setting up some of our events.