fix(monitor): provide always valid exitcode

[jusito]

Description

These changes are the core changes needed for my docker pull request. The changes are tested against every servercode.

1. command monitor had multiple issues, mainly it doesn't exit with a usable exit code. I applied the style guide and documentation should be correct now.
2. 127.0.0.1 should be part of fallback ips and maybe the others should even be removed. E.g. the port config inside a container can be 27015 but its mapped to host 27016, maybe because on 27015 is another server. Therefore if the host ip is taken to do a check, the wrong server is monitored.
3. lgsm/functions/core_dl.sh:368 hides curl failing
4. linuxgsm.sh changes are really useful to debug multiple invocations. As a result you can run a clean container and dump this file after the run to get a complete debug log.
5. lgsm/functions/install_server_files.sh missed a check if branch is missing
6. lgsm/functions/fix.sh refactoring to get rid of lots of branches which are error prone. Also added check to validate names added to arrays are valid.
7. lgsm/functions/install_header.sh probably a typo?

Fixes #[issue]

Type of change

• Bug fix (a change which fixes an issue).
• New feature (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 draft pr which depends on this.
• 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 with 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.

• User docs: https://github.com/GameServerManagers/LinuxGSM-Docs
• Dev docs: https://github.com/GameServerManagers/LinuxGSM-Dev-Docs

Thank you for your Pull Request!

[dgibbs64]

Some interesting changes. I will have to review further myself and make any further changes required. Good PR. Thanks

[dgibbs64]

This is required for logging

[dgibbs64]

Nice improvement :)

[dgibbs64]

removing script_log_error removed logging for this

[jusito]

What is your rule for using fn_script_log_* here ?
Do you prefer setting the exitcode by fn_scrip_log_* in this case I can add it also in the other locations where I set exitcode directly.

I didn't add it because it seems like duplicate log output, if there would be a fn_script_log_dots [and replacing in monitor fn_print_dots with it] or fn_print_*_nl() which also setting the exitcode, I would be more satisfied. Would you be fine with such a solution?

[dgibbs64]

LinuxGSM would fail long before getting here is $shortname was was not available

[jusito]

bdserver will run into this without the fix from following pr, makes it more safe to use here.
https://github.com/jusito/LinuxGSM/blob/fix/bdserver/lgsm/functions/install_server_files.sh#L13-L16

[jusito]

Some interesting changes. I will have to review further myself and make any further changes required. Good PR. Thanks

You are welcome, regarding the changes I would suggest to guide me, because there are around 5-20 other pull requests waiting to be opened after this. Do you prefer a pr per servercode even if only one file is changed or group some of them. E.g. some servers just don't listen to serverquery port, even in an isolated environment with correct config and correct invocation. Therefore I tried to use another mode or set it to "1".

[jusito]

Resolved the conflicts, messed a bit with git history and needed to rewrite it.
Just to be sure everything is working iam running my own tests on all gameservers and checking the logs, please dont merge until these tests are done.

[jusito]

test logs looking good for me

[Claiyc]

Then I guess this is ready to merge @h3o66 @dgibbs64

[dgibbs64]

Looking to get this merged. However, the issue I have is there are multiple features/fixes in this PR. Ideally these fixes should be broken down in to smaller PR's that will allow me to test individually and merge.

I will have a go a breaking down the code a merging in a bit at a time

[jusito]

I would like to help you, I have time after thursday but could finish it until saturday evening, would that help you ?

[dgibbs64]

Yeah that would be awesome. I am going though another PR currently and then I'm going to break down this one. When you start breaking things down just let me know which feature you are doing so we don't overlap :)

[jusito]

• currently rebasing this pr
• reviewing it again

  • check_ip.sh, refactor / 127.0.0.1 as fallback default

    127.0.0.1 should be part of fallback ips and maybe the others should even be removed. E.g. the port config inside a container can be 27015 but its mapped to host 27016, maybe because on 27015 is another server. Therefore if the host ip is taken to do a check, the wrong server is monitored.
    Examples:

    1. Used on host directly
       • As long as 127.0.0.1 is there or only user configured ip is used, its fine.
    2. Used via docker / other virtual environment which is provided by lgsm, lets say we using virtual port mapping so port in container is 27015 but port outside is host:27016 and lets say a second server is running on host:27015.
       • Any non 127.0.0.1 ip could be an issue but we could add it on docker side into the config as "ip". The scripts respecting only 127.0.0.1 in this case and if the user is able to override it this code is fine.
    3. Like b. but a virtual environment which isn't provided by lgsm, e.g. third party docker images.
       • 127.0.0.1 is the only valid option as default, some could argue ips above that like host ip should be configured by the user. Any other ip, even as fallback, could led to weird restarts which are hard to debug.

    The last case was before and is after this still an issue. But we can't only add 127.0.0.1 untested, because some server might not accept such requests. So that would need complete testing.

  • core_dl.sh, exit code of curl overwritten before stored

  • fix.sh refactor to remove error prone branches and also added checks

  • info_messages.sh if referenced queryport / voiceport is "NOT SET" grep would look for NOT in the file SET and silently fail

  • query_gamedig.sh, should always provide "querystatus" and in particular on fail != 0 for monitor command & a typo in a variable name

  • install_server_files.sh, fail safely if serverfiles arent configured correctly

  • command_monitor.sh, looks good to me. Only the part where you would like to see logging to console and when for fn_script_ , I would need a hint for that.

• breaking it down

[jusito]

split into individual prs.

[dgibbs64]

Thanks for splitting them up. I was able to review and merge most quite quickly.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.