-
Notifications
You must be signed in to change notification settings - Fork 105
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: Docker support #89
Conversation
cp /var/config.json /var/config/config.json | ||
fi | ||
|
||
if [[ ! -z "${RUN_ONCE}" ]]; 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.
[[
is bashism, but /bin/sh
is the interpreter.
(cd ./src && python3 ./main.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.
you don't need a subshell, there's nothing else after python
command, so no need to subshell cd
'ing
in fact. you should use exec
to release memory from shell process:
(cd ./src && python3 ./main.py) | |
cd ./src && exec python3 ./main.py |
and perhaps just use set -e
instead of &&
(cd ./src && python3 ./main.py) | |
see -e | |
cd ./src | |
exec python3 ./main.py |
SCHEDULE="${CRON_SCHEDULE}" | ||
fi | ||
# install cronjob | ||
(crontab -l ; echo "$SCHEDULE python3 /var/src/main.py") | sort - | uniq - | crontab - |
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.
you did cd
in plex_track_sync.sh
, but not here. is the cd
needed or not?
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.
| sort - | uniq -
can be replaced with | sort -u
but check it first, you may have crippled sort
in your image, i.e one from (older) busybox
# install cronjob | ||
(crontab -l ; echo "$SCHEDULE python3 /var/src/main.py") | sort - | uniq - | crontab - | ||
# run crond so the container does not terminate | ||
crond -l 2 -f |
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.
if this crond
is last command in the script, prepend with exec
, so that:
- memory is released from shell
- signals get routed to main process, not shell (which typically ignores them)
I ran
Is the volume map for config/ missing in docker-compose.yml ? edit: after adding ./config:/var/config it's working good. I understand docker-compose.yml is in WIP status. |
@twolaw you need to mount “src” and “config” folder. ./src:/var/src |
As per note of @glensc in #88 this is now a pull request.
The following changes have been made:
What might be the most controversial change, is that I am rerouting the log outputs to stdout if the terminal is non-interactive. I use it to detect if the script is running in a container since Docker containers should output to stdout/stderr and let Docker take care of the logging. Do you agree the script should no longer write to
lastRun.log
if it is running in a container and is thesys.isatty(0)
check okay or should I add an argument likepython3 main.py --log-to-stdout
so this is easier to control?Since the file structure changes alone constitute some manual work when upgrading from an older version of PlexTraktSync, I also propose we create a release 0.9.0 before merging the pull request for this once the feature is complete and then release 1.0.0 along with the docker image (I'd like to try Github Packages as the image registry and create a Github action that automatically builds, tags and pushes each new release in the future)