Skip to content

Conversation

@dgibbs64
Copy link
Member

Description

Please include a summary of the change and which issues are fixed.

Fixes #[issue]

Type of change

  • Bug fix (a change which fixes an issue).
  • New feature (a change which adds functionality).
  • New Server (new server added).
  • Refactor (restructures existing code).
  • Comment update (typo, spelling, explanation, examples, etc).

Checklist

PR will not be merged until all steps are complete.

  • This pull request links to an issue.
  • This pull request uses the develop branch as its base.
  • This pull request subject follows the Conventional Commits standard.
  • This code follows the style guidelines of this project.
  • I have performed a self-review of my code.
  • I have checked that this code is commented where required.
  • I have provided a detailed enough description of this PR.
  • I have checked if documentation needs updating.

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!

- 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`.
Copilot AI review requested due to automatic review settings November 10, 2025 20:53
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 remotebuildversion to remotebuild across all update modules
  • Moves last-updated.lock timestamp 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 echo commands with standardized fn_print functions

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.lock timestamp is moved to line 102 (before the update), but there's no code to refresh localbuild after the update completes. This means when alert.sh is called at line 134, localbuild still 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.lock timestamp is now written before the actual update occurs (before fn_dl_steamcmd at lines 247 or 255), but there's no code to refresh localbuild after the update completes. This means when alert.sh is called at line 265, localbuild still 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:

  1. previousbuild="${localbuild}" is captured before the update
  2. fn_update_steamcmd_localbuild silent (or similar) is called after fn_dl_steamcmd to refresh the local build
  3. 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.


fn_alert_update_linuxgsm() {
fn_script_log_info "Sending alert: ${selfname} has received an LinuxGSM update"
fn_script_log_info "Sending alert: ${selfname} has received a LinuxGSM update"
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

Typo in the alert message: "an LinuxGSM" should be "a LinuxGSM". The article "an" is used before words beginning with vowel sounds, but "LinuxGSM" starts with a consonant sound (/l/).

Copilot uses AI. Check for mistakes.
@@ -1 +1 @@
disable=SC2154
disable=SC2154,SC2034
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

The shellcheck disable directive SC2034 has been added globally. SC2034 warns about unused variables. Adding this globally will suppress warnings about all unused variables across the entire codebase, which could hide legitimate issues. Consider adding inline # shellcheck disable=SC2034 comments only where needed (e.g., where previousbuild is intentionally set for later use) rather than disabling it globally.

Suggested change
disable=SC2154,SC2034
disable=SC2154

Copilot uses AI. Check for mistakes.
fn_script_log_fail "jq is not installed"
core_exit.sh
fi
remotelocation="factorio.com"
Copy link

Copilot AI Nov 10, 2025

Choose a reason for hiding this comment

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

The remotelocation value has been changed from "github.com" to "factorio.com". This appears to be incorrect for the JK2 (Jedi Knight II) server update module, which should be fetching updates from GitHub according to the API URL. This looks like it was accidentally changed when similar code was being modified.

Suggested change
remotelocation="factorio.com"
remotelocation="github.com"

Copilot uses AI. Check for mistakes.
* Changed "an LinuxGSM update" to "a LinuxGSM update" for grammatical accuracy.
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

…" field

* Ensures proper formatting of the Discord alert message.
* Improves the display of the server status information.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants