Overhaul of start.sh and Dockerfiles #43
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… This includes cleaning up the version extraction from the JSON response by using jq. Adding jq to the Dockerfiles to support this. Also adding vim to the Dockerfiles so that users have a choice between nano and vim.
…inecraft, the USER to minecraft, changing apt to apt-get, and removing unused build stage from the amd64 Dockerfile; shart.sh - lots and lots of changes to streamline some logic (e.g. around QuietCurl), fix bash linting warnings, logging tweaks, consolidating common curl args, suppressing verbose file output for backups, other miscellaneous tweaks
… removed sudo from the install list since we are now running as the minecraft user (which cannot sudo, that's kind of the point), alphabetizing the apt-get install lists to make auditing/checking easier, breaking the long apt-get installs into one package per line to make auditing/checking much easier, making MaxMemory= instead of just = is explicit and makes IDEs happier (plus bash doesn't have variable types, so it isn't like this was an int and now is a string), adding new minecraft user changes to all Dockerfiles
…g more things, reverting some logs from printf because they end up delayed flushing to stdout, tweaking some more logs/echos
…output during backups...it'll just be noisy and clutter the logs (albeit much less than the previous highly verbose backup logging)
|
I cannot seem to view the results of the failed tests from Docker Hub. If you can tell me why they failed, I'll gladly make adjustments. As a test, I ran the arm64v8 build locally and it seemed to build fine, so I do not have any signal as to the cause of the test failures. |
cyberops7
changed the title
|
Here is sample log output from a run targeting server |
|
Here is sample log output from a run targeting server |
|
Here is sample log output from a run with the user for the container overridden as |
|
I'll try removing the Edit - well, that's closer. I wish I could see what the test failure logs are. Please let me know why that one remaining test is failing. Are these tests flaky? |
… stages to see if that change is what broke the failing tests.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This project is a great public resource, and I wanted to give back by putting some effort into giving things a facelift.
I have tested this new image repeatedly against a couple of my test instances in kubernetes that use this project. The changes have worked well for me, there is a caveat here. The image has been running as
rootfor a long time, and recentlystart.shwas updated to have execution switch over to a newminecraftuser after changing ownership of all files to the new user.Warning
The changes here are not breaking (as far as I can tell) if people have already been running that most recent image with the
root->minecraftuser. However, if users have not previously launched with this more recent image, then all their files will still be owned byroot, and images build from this PR will not be able to change ownership (since it runs as a non-root user, it cannot change ownership of files owned byroot).There is a workaround for this: run the container once as
root. That will trigger the current (and retained) conditional block at the beginning of the script to change ownership of all files and then rerun the start script as theminecraftuser. Alternatively, users could go in and manually set ownership of everything in the/minecraftdirectory to theminecraftuser/group (uid:gid 999:999).This is easily done in a compose file by adding
user: rootto the minecraft container service. For example:Run it that way once, then afterwards you can remove that additional line.
In kubernetes, the same thing can be accomplished by defining the
spec.template.spec.securityContextfor the Deployment/StatefulSet. For example:Important
This branch is based on the other PR I submitted (#42) to update
start.shto work with the new endpoint for PaperMC's API. That one should be merged before this one.There are a lot of changes here, but I'll do my best to summarize/explain them here:
sudopackage, since it isn't needed anymoreapt updateto the preferredapt-get updatefor scriptingminecraftuser and group/minecraftdirectory (so that we can set it as the WORKDIR) and set its permissionsminecraft/minecraftso when we exec into the container, it drops us right into that directorystart.shQuietCurloverride so that each command using it is not duplicated. Duplication like that is an easy way to introduce bugs by forgetting to make a change to both commands. Replicated this same approach to similar command duplication in the script as well.findvsls -1tr, double quotes on variables to prevent globbing,||conditions when changing directories, checking for variable/env var existence before use, etc.)BackupCountto 0 will delete all backups.jqlogic to automatically fall back to check the latest experimental channel build if no default channel builds are found for the desired version. This was done to, again, remove the need for a duplicated command that could lead to bugs over time.curlarguments/flags into an array so they didn't clutter up the script as much.I think that about covers it. I am happy to discuss anything that seems confusing or why I made some of the choices I did.
Fixes #41, which is what lead me down this path anyway, since you can't actually run the container as the
minecraftuser in its current state, which, in my opinion, is the "better" way to do this vs obfuscating it within the entrypoint script.