Skip to content

Conversation

@tofran
Copy link
Contributor

@tofran tofran commented Oct 4, 2020

I have been using the script a lot, thus thank you @nicolaschan.

This pull request fixes warnings in cases where they did not make much sense:

  • WARNING: Minecraft screen name not specified (use -s)
    Even when ENABLE_CHAT_MESSAGES is false - you don't want to tell players in Minecraft chat about backup status, thus it makes sense to not provide a screen name.

  • WARNING: MAX_BACKUPS (-1) is smaller than TOTAL_BLOCK_SIZE (70)
    If the MAX_BACKUPS is set to infinite (-1) it should not output this.

What about SUPPRESS_WARNINGS?

This functionality is kept working as expected - and it remains unchanged.

@nicolaschan
Copy link
Owner

Thank you for your contribution!

For the first suggestion, a valid screen name is still required for proper functioning even if you don't want chat messages. This is because the script will run the save-off and save-on commands in the screen to disable world auto-saving during the backup. If the world auto-saves during the backup then the backup will fail or become corrupted. In short, the screen name is not only for chat messages but also to ensure world auto-saving does not cause problems with the backups. Let me know if I'm misunderstanding your intent here.

The second suggestion for the warning about MAX_BACKUPS is good.

If you remove the first change about the screen name then I'll go ahead and accept this pull request. I'm also open to continue discussing the screen name warning.

@nicolaschan nicolaschan self-assigned this Oct 4, 2020
@tofran
Copy link
Contributor Author

tofran commented Oct 4, 2020

Thank you, I didn't have a full understanding on how Minecraft saves the world.
No I understand that it is relevant, but I run the script decoupled from the server, and the server does not run in a screen... I'll have to find a solution.

For now, I reverted the first if, feel free to merge this.

@nicolaschan nicolaschan merged commit c1ff08b into nicolaschan:master Oct 4, 2020
@nicolaschan
Copy link
Owner

Thank you! In the future, I think ideally this script should use rcon to send commands to the server instead of relying on screen/tmux, but that would require adding an additional installation dependency such as mcrcon. This would allow for the backups be more decoupled for situations like yours where the server is decoupled from the backup script (such as if the server runs in a docker container).

I'm still considering how best to add this without sacrificing the ease of installation of the current script. Let me know if you have any ideas!

@tofran tofran deleted the feature/smarter-warnings branch October 4, 2020 23:11
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