Skip to content
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.

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

Refactor file structure, enhance backup manager functionality and fix permissions issues #160

Closed
wants to merge 31 commits into from

Conversation

thejcpalma
Copy link
Contributor

@thejcpalma thejcpalma commented Feb 1, 2024

Enhancements and Refactoring

Summary

This PR introduces several enhancements and changes aimed at improving the organization, readability, and functionality of the project. It includes restructuring the file structure, refactoring the code, improving log output, enhancing the backup manager, and updating the Dockerfile and README.md.

Motivation

The motivation behind these changes were to enhance the project's already great structure and functionality. I wanted to do linting on the code to ensure it adheres to best practices and standardization, allowing for the addition of the flow to check shell linting.
I also wanted to improve the log output for better readability, hence the new log messages (still to improve).
Also, to enhance the backup manager to accept arguments for multiple backup operations.
And to fix the permissions issues with the Dockerfile and the volumes, leading to users having to run chmod or chown on the host. (Effectively achieving what I wanted in #147)

Changes

  • Restructured the file structure for better organization.
  • Refactored code to best practices and to standardize it.
  • Improved output of logs for readability (with colors too!).
  • Enhanced backup manager to accept arguments for multiple backup operations.
  • Added ability to run backup manager as a pseudo binary (symlink to /usr/local/bin/).
  • Removed VOLUME and USER directives from Dockerfile to fix permissions issues.
  • Added a new entrypoint script to handle permissions and launch servermanager.sh as steam user.
  • Updated README.md to reflect changes and improve usability.
  • Added the setup of rcon.yaml to be enforced if RCON_EBABLED=true and RCON_PORT=<rcon_port_number> and ADMIN_PASSWORD=<your_admin_pw> all exist and are set, independently of the value in SERVER_SETTING_MODE.

Testing

The changes were validated by running the project locally multiple times with different setups of docker compose, volumes and named volumes and verifying permissions and correct ownership of the main script running on the image.
The refactored code was tested with ShellCheck.
The improved log output was checked for better readability on both live logging and inspection.
The enhanced backup manager was tested with multiple backup operations, during multiple stages of the container, to ensure the best usage and minimize risk scenarios.
The updated Dockerfile was used to build a Docker image and run a container to confirm the fixed permissions issues.
The new entrypoint script was executed to verify it handles permissions and launches servermanager.sh as the steam user.
The updated README.md was reviewed to ensure it accurately reflects the changes and improves usability.

Additional Information

These changes collectively aim to improve the project's overall quality and usability.
Also complete/fix #147.
Fixes #156.
Fixes #117.

…ix permissions issues"

Details:
- Restructured the file structure for better organization
- Refactored code to best practices and to standardize it
- Improved output of logs for readability (with colors too!)
- Enhanced backup manager to accept arguments for multiple backup operations
    - Added server backup create, list and clean operations (more to come)
    - Complemented @jammsen backup retention policy
- Added ability to run backup manager as a pseudo binary (symlynk to /usr/local/bin/)
    - Also to run rcon protected by the env var (if set to true, runs normally)
    - Also other commands, but now they do nothing (just to keep the structure)
- Removed VOLUME and USER directives from Dockerfile to fix permissions issues
    - Still retain main process as steam user for obvious security reasons
- Added a new entrypoint script to handle permissions and launch servermanager.sh as steam user
- Updated README.md to reflect changes and improve usability
entrypoint.sh Outdated Show resolved Hide resolved
@jammsen jammsen added the in-review This issue planned to be reviewed or in progess of review label Feb 2, 2024
@jammsen jammsen self-requested a review February 2, 2024 08:11
Copy link
Owner

@jammsen jammsen left a comment

Choose a reason for hiding this comment

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

Added my thoughts and comments.

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
scripts/servermanager.sh Show resolved Hide resolved
scripts/servermanager.sh Outdated Show resolved Hide resolved
scripts/servermanager.sh Show resolved Hide resolved
scripts/servermanager.sh Show resolved Hide resolved
scripts/servermanager.sh Outdated Show resolved Hide resolved
Copy link
Contributor Author

@thejcpalma thejcpalma left a comment

Choose a reason for hiding this comment

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

Fixed all changes requested in review with @jammsen

@thejcpalma thejcpalma requested a review from jammsen February 4, 2024 12:20
Organized default.env for better readability
Added aliases for install.sh webhook operations
scripts/servermanager.sh Outdated Show resolved Hide resolved
@thejcpalma
Copy link
Contributor Author

thejcpalma commented Feb 4, 2024

Some explanations on some things:

  • New line a the end of files -> https://stackoverflow.com/questions/2287967/why-is-it-recommended-to-have-empty-line-in-the-end-of-a-source-file
  • -passthrough-logs in supercronic -> Improves logging as it keeps supercronic job start and end and its status but improves readability of what the job prints:
    2024-02-04 16:31:00 palworld-dedicated-server  | time="2024-02-04T16:31:00Z" level=info msg=starting iteration=1 job.command="backup_manager --create" job.position=0 job.schedule="*/1 * * * *"
    2024-02-04 16:31:00 palworld-dedicated-server  | Broadcasted: 16-31-00-Saving-in-5-seconds
    2024-02-04 16:31:05 palworld-dedicated-server  | Broadcasted: Saving-world...
    2024-02-04 16:31:05 palworld-dedicated-server  | Complete Save
    2024-02-04 16:31:06 palworld-dedicated-server  | Broadcasted: Saving-done
    2024-02-04 16:31:06 palworld-dedicated-server  | Broadcasted: Creating-backup
    2024-02-04 16:31:08 palworld-dedicated-server  | Broadcasted: Backup-done
    2024-02-04 16:31:08 palworld-dedicated-server  | >> Backup 'saved-20240204_163100.tar.gz' created successfully.
    2024-02-04 16:31:08 palworld-dedicated-server  | time="2024-02-04T16:31:08Z" level=info msg="job succeeded" iteration=1 job.command="backup_manager --create" job.position=0 job.schedule="*/1 * * * *"
  • docs/ dir now has all new .md used to specify topics and help without cluttering the main README.md

Any questions just ask 😉

Example image of autobackup creation:
image

thejcpalma added a commit to thejcpalma/docker-palworld-dedicated-server that referenced this pull request Feb 6, 2024
@jammsen
Copy link
Owner

jammsen commented Feb 9, 2024

As discussed #179 continues this PR and was just merged to develop. Closing this as agreed.

@jammsen jammsen closed this Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in-review This issue planned to be reviewed or in progess of review
Projects
None yet
3 participants