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

Add cron command for DockerFile #101

Closed
wants to merge 0 commits into from

Conversation

GuillaumeBarreau
Copy link
Contributor

@GuillaumeBarreau GuillaumeBarreau commented Jan 25, 2024

Context

Setting Up Automatic Backups with Cron

Adding 3 new environment variables to automate the execution of backups on the server."

Environment Variables

DAYS_TO_KEEP:

Description: This environment variable represents the number of days to retain backup files. It is used in the cleanup process to remove backup files older than the specified duration. Example Usage: If set to 7, backup files older than 7 days will be deleted during the cleanup.

BACKUP_CRON_EXPRESSION:

Description: This environment variable defines the cron expression for scheduling automatic backups. It determines when the backup script should run at specific intervals. Example Usage: If set to 0 2 * * *, the backup script will run every day at 2:00 AM.

BACKUP_ENABLED:

Description: This environment variable indicates whether automatic backups are enabled or not. If set to a non-empty value, automatic backups will be enabled; otherwise, they will be disabled. Example Usage: If set to any non-empty string (e.g., true or 1), the backup script will be configured and executed as per the specified cron schedule.

Test instructions

You can try to set up these variables with this value if you want to save backups every minute.

      BACKUP_ENABLED=true
      BACKUP_CRON_EXPRESSION=* * * * *

Checklist before requesting a review

  • I have performed a self-review of my code
  • I've added documentation about this change to the README.
  • I've not introduced breaking changes.

@thijsvanloef
Copy link
Owner

Thanks for the PR, I will check it once I get home from work!

@thijsvanloef thijsvanloef added the enhancement New feature or request label Jan 25, 2024
Dockerfile Outdated Show resolved Hide resolved
scripts/init.sh Outdated Show resolved Hide resolved
scripts/start.sh Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
docker-compose.yml Outdated Show resolved Hide resolved
@thijsvanloef thijsvanloef changed the title feat: ✨ Add cron command for DockerFile Add cron command for DockerFile Jan 25, 2024
Copy link
Owner

@thijsvanloef thijsvanloef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested this on my local machine, it does not seem to work. I also found some major issues with the code

Dockerfile Outdated
@@ -5,12 +5,17 @@ RUN apt-get update && apt-get install -y --no-install-recommends \
xdg-user-dirs=0.17-2 \
procps=2:3.3.17-5 \
wget=1.21-1+deb11u1 \
&& apt-get install -y cron \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is already a list of packages to install, please just use cron=<version>

Dockerfile Outdated
TZ=UTC
TZ=UTC \
BACKUP_ENABLED=true \
DAYS_TO_KEEP=7 \
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would preferably set this to "BACKUPS_TO_KEEP" and not days

@@ -17,6 +17,9 @@ services:
- TZ=UTC
- ADMIN_PASSWORD="adminPasswordHere"
- COMMUNITY=false # Enable this if you want your server to show up in the community servers tab, USE WITH SERVER_PASSWORD!
- BACKUP_ENABLED=false
- DAYS_TO_KEEP=7
- BACKUP_CRON_EXPRESSION=0 0 * * *
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to be quoted

@@ -17,6 +17,9 @@ services:
- TZ=UTC
- ADMIN_PASSWORD="adminPasswordHere"
- COMMUNITY=false # Enable this if you want your server to show up in the community servers tab, USE WITH SERVER_PASSWORD!
- BACKUP_ENABLED=false
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backups are enabled by default in the Dockerfile. Here you set it to false, what is the preferred status of this setting?

scripts/start.sh Outdated
@@ -67,6 +67,21 @@ if [ -n "${RCON_PORT}" ]; then
sed -i "s/RCONPort=[0-9]*/RCONPort=$RCON_PORT/" /palworld/Pal/Saved/Config/LinuxServer/PalWorldSettings.ini
fi

if [[ -n "${BACKUP_ENABLED}" ]]; then
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be if BACKUP_ENABLED = true, now it turns backups on even if the status is false

scripts/start.sh Outdated
echo "BACKUP_CRON_EXPRESSION=${BACKUP_CRON_EXPRESSION}"
fi

echo "${BACKUP_CRON_EXPRESSION} root bash /usr/local/bin/backup" > /etc/cron.d/backups-cron
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be in the "else" clause, this will fail if BACKUP_CRON_EXPRESSION is empty

FILE_PATH="${DESTINATION_PATH}/backup_palworld_${DATE}.tar.gz"

if [ ! -f ${FILE_PATH} ]; then
echo "\e[0;32m***** CREATING BACKUPS FOLDER *****\e[0m\n"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace echo with printf

scripts/start.sh Outdated
echo "BACKUP_CRON_EXPRESSION=${BACKUP_CRON_EXPRESSION}"
fi

echo "${BACKUP_CRON_EXPRESSION} root bash /usr/local/bin/backup" > /etc/cron.d/backups-cron
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you do this as the steam user instead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants