fix (alerts): Alerts on update uses the old game server version not the new one #4854
Conversation
- Replaced direct echo statements with fn_print functions for uniformity in output formatting across multiple update scripts. - Ensured lockfile creation uses the correct syntax for variable expansion. - Moved lock file to the same location to ensure monitor doesnt reboot at the wrong time - Updated remote build information display to enhance readability and maintain consistency in logging. - Removed unnecessary echo commands to streamline the output process.
- Updated variable names from 'remotebuildversion' to 'remotebuild' for consistency. - Adjusted logic to check for remote build availability using the new variable name. - Modified output messages to reflect the change in variable naming. - Ensured all modules (update_mc.sh, update_mcb.sh, update_mta.sh, update_pmc.sh, update_ts3.sh, update_ut99.sh, update_vints.sh, update_xnt.sh) are aligned with the new naming convention for better readability and maintainability.
…title * Moved `info_distro.sh`, `info_game.sh`, and `info_messages.sh` calls to the top of the `fn_alert_log` function for better clarity. * Updated `alerticon` assignment to maintain consistency in alert information handling.
* Eliminated the "More info" field to streamline the alert message. * This change enhances the clarity of the alert by focusing on essential information.
* Updated alert messages in `fn_alert_update` and `fn_alert_update_failed` for better readability. * Changed alert action from `update-request` to `update-restart-request` in `fn_monitor_check_update_source`.
…-old-version-not-the-new-one
There was a problem hiding this comment.
Pull Request Overview
This PR attempts to fix a bug where Discord alerts (and other alerts) display the old game version instead of the new version after an update. The fix involves renaming remotebuildversion to remotebuild for consistency, moving the last-updated.lock timestamp write earlier in the update process, improving alert messages to show version transitions, and adding update verification logic.
- Standardizes variable naming from
remotebuildversiontoremotebuildacross all update modules - Moves
last-updated.locktimestamp write to before the update process begins - Enhances alert messages to show version transitions (e.g., "1.0.0 -> 1.0.1")
- Adds update verification and failure detection in
update_fctr.sh - Replaces
echocommands with standardizedfn_printfunctions
Reviewed Changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| lgsm/modules/update_*.sh (multiple) | Renamed remotebuildversion to remotebuild, moved lockfile timestamp, replaced echo with fn_print functions |
| lgsm/modules/update_fctr.sh | Added update verification, previousbuild tracking, and post-update localbuild refresh |
| lgsm/modules/core_steamcmd.sh | Updated variable naming and lockfile timing for SteamCMD-based updates |
| lgsm/modules/alert.sh | Enhanced update alerts to show version transitions, added update-failed alert, renamed update-request alert |
| lgsm/modules/alert_discord.sh | Removed redundant "More info" field from Discord embeds |
| lgsm/modules/command_monitor.sh | Updated alert type name from update-request to update-restart-request |
| lgsm/modules/check_last_update.sh | Added blank line for formatting |
| .shellcheckrc | Added SC2034 to global disable list |
Comments suppressed due to low confidence (2)
lgsm/modules/update_vints.sh:134
- The
last-updated.locktimestamp is moved to line 102 (before the update), but there's no code to refreshlocalbuildafter the update completes. This means whenalert.shis called at line 134,localbuildstill contains the old version, not the newly installed version. This is the bug described in the PR title - alerts will show the old version instead of the new one.
Consider following the pattern in update_fctr.sh (lines 103-104 and 133) where previousbuild is captured before the update and fn_update_localbuild is called after the download to refresh the local build value.
if [ "${commandname}" == "UPDATE" ]; then
date +%s > "${lockdir:?}/last-updated.lock"
unset updateonstart
check_status.sh
# If server stopped.
if [ "${status}" == "0" ]; then
fn_update_dl
if [ "${localbuild}" == "0" ]; then
exitbypass=1
command_start.sh
fn_firstcommand_reset
exitbypass=1
fn_sleep_time_5
command_stop.sh
fn_firstcommand_reset
fi
# If server started.
else
fn_print_restart_warning
exitbypass=1
command_stop.sh
fn_firstcommand_reset
exitbypass=1
fn_update_dl
exitbypass=1
command_start.sh
fn_firstcommand_reset
fi
unset exitbypass
alert="update"
elif [ "${commandname}" == "CHECK-UPDATE" ]; then
alert="check-update"
lgsm/modules/core_steamcmd.sh:265
- The
last-updated.locktimestamp is now written before the actual update occurs (beforefn_dl_steamcmdat lines 247 or 255), but there's no code to refreshlocalbuildafter the update completes. This means whenalert.shis called at line 265,localbuildstill contains the old version, not the newly installed version. This is exactly the bug described in the PR title - alerts will show the old version instead of the new one.
Consider following the pattern in update_fctr.sh (lines 103-104 and 133) where:
previousbuild="${localbuild}"is captured before the updatefn_update_steamcmd_localbuild silent(or similar) is called afterfn_dl_steamcmdto refresh the local build- This ensures alerts show the correct new version
date +%s > "${lockdir:?}/last-updated.lock"
unset updateonstart
check_status.sh
# If server stopped.
if [ "${status}" == "0" ]; then
fn_dl_steamcmd
# If server started.
else
fn_print_restart_warning
exitbypass=1
command_stop.sh
fn_firstcommand_reset
exitbypass=1
fn_dl_steamcmd
exitbypass=1
command_start.sh
fn_firstcommand_reset
fi
unset exitbypass
alert="update"
elif [ "${commandname}" == "CHECK-UPDATE" ]; then
alert="check-update"
fi
alert.sh
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* Changed "an LinuxGSM update" to "a LinuxGSM update" for grammatical accuracy.
…" field * Ensures proper formatting of the Discord alert message. * Improves the display of the server status information.
* Changed the order of fields in the Discord alert JSON structure. * Updated the "More info" and "Is my Game Server Online?" fields for better clarity.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Ensures consistency in alert messages across Discord, NTFY, Pushover, Rocketchat, Slack, and Telegram. Updates Discord alerts to include server hostname and time, removes redundant formatting. Standardizes plain-text alert formatting for NTFY. Refactors Pushover messages to use consistent HTML formatting. Streamlines Rocketchat and Slack messages by removing duplicate server info and adding optional link support.
Removes the Hostname field from various alert messages (Discord, Gotify, IFTTT, ntfy, Pushbullet, Pushover, Rocketchat, Slack, Telegram). The hostname was deemed redundant or unnecessary in the context of these alerts. Relates to #4696
dgibbs64
changed the title
…-old-version-not-the-new-one
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| "title": "${alerttitle}", | ||
| "message": "Server Name\n${servername}\n\nInformation\n${alertmessage}\n\nGame\n${gamename}\n\nServer IP\n${alertip}:${port}\n\nHostname\n${HOSTNAME}\n\n | ||
| "message": "Server Name\n${servername}\n\nInformation\n${alertmessage}\n\nGame\n${gamename}\n\nServer IP\n${alertip}:${port}\n\n | ||
| EOF |
There was a problem hiding this comment.
The JSON payload is assembled by keeping the message string open across heredocs, which embeds literal newlines inside a JSON string (invalid JSON) and will make jq -c . fail. Please build the full message string separately (with explicit \n escapes) and then include it as a single quoted JSON string, or construct the JSON using jq -n --arg message ....
| # If previousbuild is set show transition, else fallback to single version. | ||
| if [ -n "${previousbuild:-}" ] && [ -n "${localbuild:-}" ]; then | ||
| fn_script_log_info "Sending alert: ${selfname} updated: ${previousbuild} -> ${localbuild}" | ||
| alertmessage="${selfname} updated: ${previousbuild} -> ${localbuild}." |
There was a problem hiding this comment.
fn_alert_update() formats the update alert using ${previousbuild} -> ${localbuild} (or just ${localbuild}), but most update modules in this PR still call alert.sh with localbuild set to the pre-update version and never set previousbuild (only update_fctr.sh does). This means update alerts will still report the old version (or won’t show a transition) for most servers. Consider using ${localbuild} -> ${remotebuild} when remotebuild is available, or standardize all update paths to set previousbuild before updating and refresh localbuild after updating before calling alert.sh.
| # If previousbuild is set show transition, else fallback to single version. | |
| if [ -n "${previousbuild:-}" ] && [ -n "${localbuild:-}" ]; then | |
| fn_script_log_info "Sending alert: ${selfname} updated: ${previousbuild} -> ${localbuild}" | |
| alertmessage="${selfname} updated: ${previousbuild} -> ${localbuild}." | |
| # Prefer previousbuild -> localbuild when available. | |
| if [ -n "${previousbuild:-}" ] && [ -n "${localbuild:-}" ]; then | |
| fn_script_log_info "Sending alert: ${selfname} updated: ${previousbuild} -> ${localbuild}" | |
| alertmessage="${selfname} updated: ${previousbuild} -> ${localbuild}." | |
| # If previousbuild is not set but remotebuild is available, show localbuild -> remotebuild. | |
| elif [ -z "${previousbuild:-}" ] && [ -n "${localbuild:-}" ] && [ -n "${remotebuild:-}" ]; then | |
| fn_script_log_info "Sending alert: ${selfname} updated: ${localbuild} -> ${remotebuild}" | |
| alertmessage="${selfname} updated: ${localbuild} -> ${remotebuild}." | |
| # If only remotebuild is available, fall back to showing the updated-to version. | |
| elif [ -n "${remotebuild:-}" ]; then | |
| fn_script_log_info "Sending alert: ${selfname} updated to ${remotebuild}" | |
| alertmessage="${selfname} updated to ${remotebuild}." |
| # Verify the update applied correctly unless forced update to same version. | ||
| if [ "${localbuild}" != "${remotebuild}" ]; then | ||
| # If forced update and version unchanged treat as acceptable; otherwise failure. | ||
| if [ "${forceupdate}" != "1" ] || [ "${previousbuild}" = "${localbuild}" ]; then | ||
| fn_script_log_error "Update verification failed: expected ${remotebuild}, got ${localbuild}" | ||
| fn_print_fail_nl "Update verification failed: expected ${remotebuild}, got ${localbuild}" | ||
| updatefailureexpected="${remotebuild}" | ||
| updatefailuregot="${localbuild}" | ||
| alert="update-failed" | ||
| alert.sh | ||
| core_exit.sh | ||
| fi |
There was a problem hiding this comment.
The update verification condition is inconsistent with the comment and can incorrectly treat a failed update as success when forceupdate==1. In the localbuild != remotebuild case, the current logic only fails if forceupdate!=1 or previousbuild==localbuild, meaning a forced update that results in an unexpected (still wrong) localbuild different from both previousbuild and remotebuild will be considered successful. It would be safer to fail whenever localbuild != remotebuild after the update (and handle the “forced update to same version” case separately, e.g., by comparing previousbuild and remotebuild before running the update).
| # Verify the update applied correctly unless forced update to same version. | |
| if [ "${localbuild}" != "${remotebuild}" ]; then | |
| # If forced update and version unchanged treat as acceptable; otherwise failure. | |
| if [ "${forceupdate}" != "1" ] || [ "${previousbuild}" = "${localbuild}" ]; then | |
| fn_script_log_error "Update verification failed: expected ${remotebuild}, got ${localbuild}" | |
| fn_print_fail_nl "Update verification failed: expected ${remotebuild}, got ${localbuild}" | |
| updatefailureexpected="${remotebuild}" | |
| updatefailuregot="${localbuild}" | |
| alert="update-failed" | |
| alert.sh | |
| core_exit.sh | |
| fi | |
| # Verify the update applied correctly: local and remote builds must match. | |
| if [ "${localbuild}" != "${remotebuild}" ]; then | |
| fn_script_log_error "Update verification failed: expected ${remotebuild}, got ${localbuild}" | |
| fn_print_fail_nl "Update verification failed: expected ${remotebuild}, got ${localbuild}" | |
| updatefailureexpected="${remotebuild}" | |
| updatefailuregot="${localbuild}" | |
| alert="update-failed" | |
| alert.sh | |
| core_exit.sh |
| @@ -44,14 +45,14 @@ fn_update_remotebuild() { | |||
| else | |||
| remotebuildversion="${branch}" | |||
| fi | |||
There was a problem hiding this comment.
remotebuild is used as the jq lookup key here, but it is never assigned in fn_update_remotebuild() (the code above still sets remotebuildversion=...). As a result these lookups will query .[] with an empty/null key and remotebuildfilename/url/hash will be wrong (and the later ${remotebuild} null-check will fail). Set remotebuild="${remotebuildversion}" (or rename remotebuildversion to remotebuild) before these jq calls and use the same variable consistently throughout the module.
| fi | |
| fi | |
| remotebuild="${remotebuildversion}" |
| "parse_mode": "HTML", | ||
| "disable_notification": "${telegramdisablenotification}", | ||
| "text": "<b>${alerttitle}</b>\n\n<b>Server name</b>\n${servername}\n\n<b>Information</b>\n${alertmessage}\n\n<b>Game</b>\n${gamename}\n\n<b>Server IP</b>\n${alertip}:${port}\n\n<b>Hostname</b>\n${HOSTNAME}\n\n | ||
| "text": "<b>${alerttitle}</b>\n\n<b>Server Name</b>\n${servername}\n\n<b>Information</b>\n${alertmessage}\n\n<b>Game</b>\n${gamename}\n\n<b>Server IP</b>\n${alertip}:${port}\n\n | ||
| EOF |
There was a problem hiding this comment.
The JSON payload is built by leaving the text string unterminated across multiple heredocs, which inserts literal newline characters into an open JSON string. jq -c . will fail to parse this with an “invalid string / control characters must be escaped” error. Build the message body in a separate variable (using \n escapes via printf) and then inject it as a fully-quoted JSON string (or generate the JSON with jq -n --arg text ...).
| "type": "note", | ||
| "title": "${alerttitle}", | ||
| "body": "Server Name\n${servername}\n\nInformation\n${alertmessage}\n\nGame\n${gamename}\n\nServer IP\n${alertip}:${port}\n\nHostname\n${HOSTNAME}\n\n | ||
| "body": "Server Name\n${servername}\n\nInformation\n${alertmessage}\n\nGame\n${gamename}\n\nServer IP\n${alertip}:${port}\n\n | ||
| EOF |
There was a problem hiding this comment.
The JSON payload construction leaves the body string open across heredocs, which causes literal newlines to be embedded in an unescaped JSON string. This makes the JSON invalid and will cause jq -c . to error. Consider building the full message string separately (with explicit \n escapes) and then include it as a single JSON string value (or generate the JSON with jq -n --arg body ...).
| "value1": "${selfname}", | ||
| "value2": "${alerttitle}", | ||
| "value3": "Server Name\n${servername}\n\nInformation\n${alertmessage}\n\nGame\n${gamename}\n\nServer IP\n${alertip}:${port}\n\nHostname\n${HOSTNAME}\n\n | ||
| "value3": "Server Name\n${servername}\n\nInformation\n${alertmessage}\n\nGame\n${gamename}\n\nServer IP\n${alertip}:${port}\n\n | ||
| EOF |
There was a problem hiding this comment.
This JSON construction leaves the value3 string unterminated across multiple heredocs, which inserts literal newline characters into an open JSON string. jq -c . will not parse that JSON. Build the message body separately (using printf with \n escapes) and then set value3 in one shot (or generate the whole payload via jq -n --arg value3 ...).
Configures the Super-Linter action to use `.shellcheckrc` as the BASH_FILE_NAME. This ensures that shellcheck uses the correct configuration file for bash script linting. Fixes #4696
…-uses-the-old-version-not-the-new-one' into 4696-bug-discord-alert-on-update-uses-the-old-version-not-the-new-one
Disables specific shellcheck errors to allow certain coding patterns and removes an unused variable from the super-linter workflow. This improves the linter's signal-to-noise ratio and reduces unnecessary checks. Relates to #4696
Addresses issues in the server list validation scripts: - Fixes potential issues with parsing curl custom arguments in telegram alerts. - Corrects the shortname array generation. - Implements more robust checks for validating game icons. - Ensures the consistency of server counts across different CSV files.
Corrects glibc version comparison to properly handle version strings. Improves mod installation and removal by validating user input against available options, providing a more robust selection process. This prevents errors caused by invalid mod names and enhances the user experience. Updates arithmetic expression syntax for better compatibility.
Updates the handling of cache control headers in dev-debug mode. The `nocache` variable is changed to an array to correctly pass multiple headers to `curl`. This resolves issues with cached versions of files being used when dev-debug is enabled, ensuring that the latest versions are always fetched. Fixes #4696
Parses steamcmdcommand as an array to allow spaces in the variable. This prevents issues when the user wants to pass arguments with spaces to the steamcmd executable.
Addresses multiple improvements across various scripts: - Fixes Valheim unstripped_corlib override by commenting out the lines in the config files. - Improves process identification for source and goldsrc engines using `pgrep`. - Enhances backup file identification using `find` with `-maxdepth 1` and `-type f`. - Fixes glibc version comparison logic in multiple files. - Improves UT2K4 key installation script by using printf for writing the key to the file. - Corrects the mod info extraction logic to correctly identify mod entries. - Improves amxmodx file installation/removal logic to prevent duplicate entries. - Fixes server info retrieval to handle spaces in the server list. Relates to #4696
Corrects the mono repo install exit code check to use the dedicated monorepoexitcode variable, ensuring accurate error detection. Updates the exit trap to preserve the original exit status, preventing potential loss of information when handling script termination.
Improves module loading by reordering and refactoring the core modules. This change addresses inconsistencies and improves the overall structure of how core modules are handled within the system. Some fix scripts were renamed to match the module name.
Changes file permissions to make shell scripts executable. This ensures that the scripts can be run directly. Addresses a pre-existing issue. Related to #4696
Specifies explicit permissions for GitHub Actions workflows to enhance security and control access to resources. This ensures that each workflow only has the necessary permissions, following the principle of least privilege.
Corrects output redirection in GitHub Actions workflows by enclosing the $GITHUB_OUTPUT variable in quotes, preventing potential issues with variable expansion. Ensures proper URL parsing in workflows by enclosing the URL within quotes, addressing potential parsing errors. Fixes #4696
Applies Prettier formatting to the codebase for improved readability and consistency. Addresses minor code style inconsistencies across several files.
Addresses various minor issues including: - Standardizes indentation in `.editorconfig` for different file types. - Simplifies Prettier configuration. - Updates image links in `README.md`. - Ensures newline character at end of `.csv` files. - Corrects a conditional statement in `command_fastdl.sh`. - Corrects a conditional statement in `info_messages.sh`. - Updates server query protocols in `query_gsquery.py`.
Adds YAML linting configuration and disables unnecessary linters to streamline the CI/CD process and reduce noise from irrelevant checks.
Updates documentation links to be enclosed in angle brackets, preventing markdown rendering issues. Configures markdownlint to align with repository standards and avoid common false positives. The updated links in the documentation ensure they are correctly interpreted by markdown parsers, improving user experience. The markdownlint configuration fine-tunes linting rules to better match the project's existing style and conventions, reducing noise from irrelevant warnings.
Addresses various typos and inconsistencies across multiple files, enhancing code readability and maintainability. Adds codespell and flake8 configurations for linting. Relates to #4696
Prevents Prettier from formatting workflow files due to GitHub token restrictions, as the token used by the Prettier auto-commit action lacks permissions to update files within the `.github/workflows/` directory.
Improves Super-Linter's reliability by switching to linting the entire codebase instead of relying on git history. This change also addresses potential transient fetch failures from GitHub. Additionally, this commit expands the ignored paths in codespell and disables some linters to improve performance and reduce false positives.
1 participant
Description
Please include a summary of the change and which issues are fixed.
Fixes #[issue]
Type of change
Checklist
PR will not be merged until all steps are complete.
developbranch as its base.Documentation
If documentation does need updating either update it by creating a PR (preferred) or request a documentation update.
Thank you for your Pull Request!