Move countdown to function for auto update and auto reboot #392

Dashboy1998 · 2024-02-15T05:45:35Z

Context

Due to all the changes in Introduced Utility Functions - Remove duplicate code snippets #356 this now has the features for/from the following
- Use DoExit instead of Shutdown #395
- Add save and shutdown functions #398
- Create broadcast function #400
- Auto reboot and auto update when players leave #393
- Removed use of $? in favor of if command
Auto update outputs a single message to the server saying it will update in N minutes whereas auto reboot outputs a message counting down N minutes.

Choices

Changed to match behavioral of auto reboot.
Due to the massive merge of Introduced Utility Functions - Remove duplicate code snippets #356 I had to implement other fixes.

Test instructions

Auto reboot

Set AUTO_REBOOT_CRON_EXPRESSION close to your current time
Set AUTO_REBOOT_ENABLED to true
Set AUTO_REBOOT_EVEN_IF_PLAYERS_ONLINE to true
Set AUTO_REBOOT_WARN_MINUTES to 5

Players in game and messages

Join the server and verify that the server restarts in 5 minutes after the cronjob has started and you receive messages for 5, 2, 1 minutes, and 30, 10 seconds.

Players leave game during countdown

Join the server and once the cronjob starts leave the game and verify it restarts on the minute after you left.

Auto Update

Set UPDATE_CRON_EXPRESSION close to your current time
Set AUTO_UPDATE_ENABLED to true
Set AUTO_REBOOT_WARN_MINUTES to 5
Set UPDATE_ON_BOOT to true
After the server starts change the buildId on line 33 in the file `palworld\steamapps\appmanifest_2394010.acf`` to any other value (for example 63707356556294349890)

Players in game and messages

Join the server and verify that the server restarts in 5 minutes after the cronjob has started and you receive messages for 5, 2, 1 minutes, and 30, 10 seconds.

Players leave game during countdown

Join the server and once the cronjob starts leave the game and verify it restarts on the minute after you left.

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.

Luatan · 2024-02-15T22:06:18Z

Test instructions

Set UPDATE_CRON_EXPRESSION to */5 * * *

needs to be "*/5 * * * *"

Set WARN_MINUTES to 2

check in the log that the cronjob is running correctly

change the buildId on line 11 in the file `palworld\steamapps\appmanifest_2394010.acf`` to any other value (for example 13289561)

I think you mean to change the manifestid on line 32 right?

Then it works 😊

Do we really want to send an notification every minute? I think that's a bit too many if the default is 30 minutes.
My suggestion would be to have set intervals. For example:
30 15 10 5 2 1

And if the auto_reboot and auto_update use this, I think it would also be a good idea to make a helper function for it.

Dashboy1998 · 2024-02-16T01:49:53Z

Test instructions

Set UPDATE_CRON_EXPRESSION to */5 * * *

needs to be "*/5 * * * *"

Set WARN_MINUTES to 2

check in the log that the cronjob is running correctly

change the buildId on line 11 in the file `palworld\steamapps\appmanifest_2394010.acf`` to any other value (for example 13289561)

I think you mean to change the manifestid on line 32 right?

Then it works 😊

Do we really want to send an notification every minute? I think that's a bit too many if the default is 30 minutes.
My suggestion would be to have set intervals. For example:
30 15 10 5 2 1

And if the auto_reboot and auto_update use this, I think it would also be a good idea to make a helper function for it.

Ah the mistakes when I copy the instructions from another PR.

I just did every minute because that's what the auto reboot does.

I can add a case statement so it's not every minute.

scripts/helper_functions.sh

scripts/auto_reboot.sh

Co-authored-by: Luatan <[email protected]>

… if players are in the server.

… is invalid

Luatan · 2024-02-18T02:04:08Z

scripts/helper_functions.sh

+ for ((i = "${mtime}" ; i > 0 ; i--)); do
+ if [ "$(get_player_count)" -eq 0 ]; then
+ break
+ fi
+ if [ "$i" -eq 1 ]; then
+ broadcast_command "${message_prefix}_in_${i}_minute"
+ sleep 30s
+ broadcast_command "${message_prefix}_in_30_seconds"
+ sleep 20s
+ broadcast_command "${message_prefix}_in_10_seconds"
+ sleep 10s
+ else
+ case "$i" in
+ "$mtime" )
+ ;&
+ 15 )
+ ;&
+ 10 )
+ ;&
+ 5 )
+ ;&
+ 2 )
+ broadcast_command "${message_prefix}_in_${i}_minutes"
+ ;&
+ * ) 
+ sleep 1m
+ ;;
+ esac
+ fi
+ done
+ return 0
+ else
+ # mtime is not an integer so check if there are no players
+ if [ "$(get_player_count)" -gt 0 ]; then
+ return_val=1
+ fi
+ fi


I would suggest to negate if [[ "${mtime}" =~ ^[0-9]+$ ]]; then to reduce nested code.

I have that because the only time this is called is during a shutdown for update or reboot.
I believe both of those should happen regardless if there's a timer or not as the timer is just to let players know. If there is no timer set and there are no players I believe the action should proceed.

Luatan · 2024-02-18T02:14:13Z

scripts/update.sh

+ if [[ "${AUTO_UPDATE_WARN_MINUTES}" =~ ^[0-9]+$ ]]; then
+ DiscordMessage "Server will update in ${AUTO_UPDATE_WARN_MINUTES} minutes"
+ fi


I think we would need to cancel the update if WARN_MINUTES is not an integer.

Suggested change

if [[ "${AUTO_UPDATE_WARN_MINUTES}" =~ ^[0-9]+$ ]]; then

DiscordMessage "Server will update in ${AUTO_UPDATE_WARN_MINUTES} minutes"

fi

if [[ "${AUTO_UPDATE_WARN_MINUTES}" =~ ^[0-9]+$ ]]; then

LogError "AUTO_UPDATE_WARN_MINUTES is not an integer: ${AUTO_UPDATE_WARN_MINUTES}. Please check your environment variables"

DiscordMessage "AUTO_UPDATE_WARN_MINUTES is not an integer: ${AUTO_UPDATE_WARN_MINUTES}. Please check your environment variables" "failure"

exit 1

fi

DiscordMessage "Server will update in ${AUTO_UPDATE_WARN_MINUTES} minutes"

I have it like that because if you so choose you could run the update script with no one in the server and it will update.

Also if it is a cron job and there is no one why not just update? The timer is supposed to just let players know and not prevent the update as that's what the enable flag is for

I see what you mean. For me it just seems like we are allowing the user to misconfigure the env variables, which feels wrong. I think we should tell the user to fix it, but maybe we should do that in the start.sh then. To do it immediately and not wait until the update / reboot takes place.

If the user wants to have the server restart immediately they could just set WARN_MINUTES to 0 (not sure if that works as expected, haven't tested that).

We should still be checking in backup.sh, restore.sh, and update.sh as those are also intended to be started by the user and not just cron.

I would say for another PR we add validation for all variables which are an expected type such as int, float, boolean.

The implementation in 0.26.0 is no validation for AUTO_UPDATE_WARN_MINUTES.

Thinking about it some more auto update and auto reboot should be their own script and then have a separate scripts which are to be ran by the user.

Because in my mind the user should be able to run update and tell it how long to wait or reboot instantly whereas now you can run it but you don't set the values.

This should definitely be a different PR.

Wouldn't it be better, if we added arg flags to the script instead of doing it interactively? Or both. Then we can call the script with the flags from the cronjob and if the user does not supply any flags it asks for them. That way we only have to manage one file.
Just an idea

I was thinking the user interactive script would just be a wrapper for the non interactive script.

scripts/update.sh

scripts/start.sh

Luatan · 2024-02-18T02:31:01Z

scripts/auto_reboot.sh

+ elif [ -z "${AUTO_REBOOT_WARN_MINUTES}" ]; then
+ LogError "Unable to auto reboot, the server is not empty and AUTO_REBOOT_WARN_MINUTES is empty"
+ exit 1
+ else
+ LogError "Unable to auto reboot, the server is not empty and AUTO_REBOOT_WARN_MINUTES is not an integer: ${AUTO_REBOOT_WARN_MINUTES}"
+ fi


I think you can do these checks before you start the countdown. Would make it a bit cleaner in my opinion.

I have it like that because if you so choose you could run the reboot script with no one in the server and it will reboot.

Also if it is a cron job and there is no one why not just reboot? The timer is supposed to just let players know and not prevent the reboot as that's what the enable flag is for

Luatan · 2024-02-18T02:36:37Z

Nice work!

(#356) this now has the features for/from the following

Use DoExit instead of Shutdown #395

Remove the need of restart policy for auto update and auto reboot #399

As far as I can tell a reboot policy is is still needed for the container both reboot and update. right?

Co-authored-by: Luatan <[email protected]>

Dashboy1998 · 2024-02-18T02:52:01Z

Nice work!

(#356) this now has the features for/from the following

Use DoExit instead of Shutdown #395

Remove the need of restart policy for auto update and auto reboot #399

As far as I can tell a reboot policy is is still needed for the container both reboot and update. right?

Yes.

scripts/helper_functions.sh

Dashboy1998 · 2024-02-19T23:17:21Z

This has turned into a very big PR. I am going to break into smaller ones.

Dashboy1998 · 2024-02-20T00:36:55Z

Still need a PR for countdown function however this will be after #411 and #412

Dashboy1998 added 3 commits February 14, 2024 12:57

Fixed spacing

019fe45

Added countdown loop

9d6b106

Added validation for AUTO_UPDATE_WARN_MINUTES

fd1a2de

Dashboy1998 added 2 commits February 15, 2024 22:47

Moved countdown to helper function and added seconds

595f0a3

Removed/Added extra lines

ed56e0f

Dashboy1998 changed the title ~~Auto Update add countdown~~ Move countdown to function for auto update and auto reboot Feb 16, 2024

Luatan reviewed Feb 16, 2024

View reviewed changes

scripts/helper_functions.sh Outdated Show resolved Hide resolved

scripts/helper_functions.sh Outdated Show resolved Hide resolved

scripts/auto_reboot.sh Outdated Show resolved Hide resolved

scripts/auto_reboot.sh Show resolved Hide resolved

Dashboy1998 and others added 19 commits February 16, 2024 12:59

Update scripts/helper_functions.sh

98f9c1e

Co-authored-by: Luatan <[email protected]>

Update scripts/helper_functions.sh

2d34179

Co-authored-by: Luatan <[email protected]>

Implemented skip when empty, resolves thijsvanloef#393

22683fd

Moved in_ to countdown message

8bef267

Moved integer validation to countdown

5471fb4

Fixed spacing in auto_reboot.sh

58a611c

Update auto_reboot so AUTO_REBOOT_WARN_MINUTES needs to only be valid…

7e54379

… if players are in the server.

Split messages in update

b79edce

No longer creating update discord message if AUTO_UPDATE_WARN_MINUTES…

80d4b1c

… is invalid

Updated messages in update.sh

e73c9f2

Updated messages in update and reboot

098818a

Merge branch 'main' into update-countdown

600313f

Added save function

25450e2

Added shutdown function

5fe4b8d

Added save to shutdown function

fa9735c

Changed mtime, message_prefix, return_val to local vars

2c8dbf6

Removed rcon-cli save/shutdown in init.sh

396a9fb

Fixed shutdown

ace7b18

Merge branch 'Add-save-and-shutdown-functions' into update-countdown

5962579

Dashboy1998 marked this pull request as draft February 17, 2024 18:57

Merge branch 'main' into update-countdown

dc66c8b

Dashboy1998 added 2 commits February 17, 2024 15:38

Fixed update/install in start

e3ae137

Added broadcast function. Partially Resolves thijsvanloef#400

b089211

Dashboy1998 marked this pull request as ready for review February 17, 2024 20:54

Luatan reviewed Feb 18, 2024

View reviewed changes

Update scripts/start.sh

82536b4

Co-authored-by: Luatan <[email protected]>

Dashboy1998 added 8 commits February 17, 2024 22:30

Changed exit codes for countdown_message

131638d

Moved player count check

958a930

Moved case when 1

1baeb04

Added comment

27c1466

Reordered cases

3e38a6e

Merge branch 'main' into update-countdown

9fb677c

Merge branch 'main' into update-countdown

5d72e7c

Checking if broadcast contains non-ascii characters.

a718120

Dashboy1998 commented Feb 19, 2024

View reviewed changes

scripts/helper_functions.sh Outdated Show resolved Hide resolved

Reverted spacing changes

408617b

Dashboy1998 marked this pull request as draft February 19, 2024 20:48

Dashboy1998 added 5 commits February 19, 2024 15:50

Added missing fi

43ca7a2

Moved source helper_install.sh out of helper_functions.sh

acdf259

Merge branch 'main' into update-countdown

e58143e

Only broadcast valid messages

f96ef24

Fixed merge issue

635d201

Dashboy1998 closed this Feb 19, 2024

This was referenced Feb 20, 2024

Create broadcast function #411

Merged

Added shutdown and save functions #412

Merged

Moved source includes #413

Merged

Dashboy1998 mentioned this pull request Feb 22, 2024

Skip count down when players left #428

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move countdown to function for auto update and auto reboot #392

Move countdown to function for auto update and auto reboot #392

Dashboy1998 commented Feb 15, 2024 •

edited

Luatan commented Feb 15, 2024

Test instructions

Dashboy1998 commented Feb 16, 2024

Test instructions

Luatan Feb 18, 2024

Dashboy1998 Feb 18, 2024

Luatan Feb 18, 2024

Dashboy1998 Feb 18, 2024

Luatan Feb 18, 2024 •

edited

Dashboy1998 Feb 18, 2024

Dashboy1998 Feb 18, 2024

Luatan Feb 18, 2024

Dashboy1998 Feb 18, 2024

Luatan Feb 18, 2024

Dashboy1998 Feb 18, 2024

Luatan commented Feb 18, 2024 •

edited

Dashboy1998 commented Feb 18, 2024

Dashboy1998 commented Feb 19, 2024

Dashboy1998 commented Feb 20, 2024

Move countdown to function for auto update and auto reboot #392

Move countdown to function for auto update and auto reboot #392

Conversation

Dashboy1998 commented Feb 15, 2024 • edited

Context

Choices

Test instructions

Auto reboot

Players in game and messages

Players leave game during countdown

Auto Update

Players in game and messages

Players leave game during countdown

Checklist before requesting a review

Luatan commented Feb 15, 2024

Test instructions

Dashboy1998 commented Feb 16, 2024

Test instructions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luatan Feb 18, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luatan commented Feb 18, 2024 • edited

Dashboy1998 commented Feb 18, 2024

Dashboy1998 commented Feb 19, 2024

Dashboy1998 commented Feb 20, 2024

Dashboy1998 commented Feb 15, 2024 •

edited

Luatan Feb 18, 2024 •

edited

Luatan commented Feb 18, 2024 •

edited