Skip to content

Add mc_health_extra_args to autopause-fcns.sh functions #2832

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Add mc_health_extra_args to autopause-fcns.sh functions #2832

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions files/auto/autopause-fcns.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,13 @@ rcon_client_exists() {
}

mc_server_listening() {
mc-monitor status --host localhost --port "$SERVER_PORT" --timeout 10s >& /dev/null
mc-monitor status "${MC_HEALTH_EXTRA_ARGS[@]}" --host localhost --port "$SERVER_PORT" --timeout 10s >& /dev/null
Copy link
Owner

Choose a reason for hiding this comment

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

The idea of this change is great, but I don't the think the syntax or usage is correct. Have you tested this?

MC_HELATH_EXTRA_ARGS is being used as an array type since normally it is declared in /data/.mc-health.env here

docker-minecraft-server/scripts/start-finalExec

Lines 226 to 229 in 7419a66

MC_HEALTH_EXTRA_ARGS=(
--use-server-list-ping
)
" > /data/.mc-health.env

however, the file .mc-health.env file doesn't seem to be getting source'd in the autopause scripts. So that needs to be added, such as this

Setting the env var MC_HEALTH_EXTRA_ARGS is not correct since env variables only have string values, not array values. So the array expansion ${MC_HEALTH_EXTRA_ARGS[@]} will not be correct.

I would suggest introducing a new env var named something like USES_PROXY_PROTOCOL that is "true" or "false". In the start-finalExec location above add a conditional on isTrue "$USES_PROXY_PROTOCOL" and add --use-proxy to MC_HEALTH_EXTRA_ARGS there.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, before making this pull request I've tested it with the --use-proxy option but that was about it.

Copy link
Author

Choose a reason for hiding this comment

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

If you wish I can make the changes you mentioned, but it will probably be in about 1.5 weeks time from now as I'm working on other projects at the moment.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, making those changes would be good. mc-health can also make use of that option then. I don't mind waiting for you to make that change.

BTW, MC_HEALTH_EXTRA_ARGS was purposely undocumented since it was only meant to communicate internally the start-time discovery for legacy ping protocol. So, it'll be great to document the new variable since it actually should be visible to users.

}

java_clients_connections() {
local connections
if java_running ; then
if ! connections=$(mc-monitor status --host localhost --port "$SERVER_PORT" --show-player-count); then
if ! connections=$(mc-monitor status "${MC_HEALTH_EXTRA_ARGS[@]}" --host localhost --port "$SERVER_PORT" --show-player-count); then
# consider it a non-zero player count if the ping fails
# otherwise a laggy server with players connected could get paused
connections=1
Expand All @@ -36,4 +36,4 @@ java_clients_connections() {

java_clients_connected() {
(( $(java_clients_connections) > 0 ))
}
}