Run docker image as non-root user #1011
Conversation
|
This is all greek to me 😄 Let me know anytime you want me to merge. |
|
Would you mind also taking a look at the docker documentation to ensure it's up to date? https://github.com/rmcrackan/Libation/blob/master/Documentation/Docker.md |
pixil98
changed the title
pixil98
force-pushed
the
master
branch
4 times, most recently
from
ccf9049 to
9bc53e4
Compare
pixil98
force-pushed
the
master
branch
4 times, most recently
from
96e5022 to
a74722e
Compare
pixil98
force-pushed
the
master
branch
5 times, most recently
from
3bf4c6b to
0179fd8
Compare
pixil98
force-pushed
the
master
branch
2 times, most recently
from
2434618 to
011efe3
Compare
pixil98
force-pushed
the
master
branch
2 times, most recently
from
8dee854 to
4f7a2ce
Compare
pixil98
force-pushed
the
master
branch
7 times, most recently
from
bd09e1e to
9e69b4a
Compare
|
I am not going to have time until November ... oof. Creating a blank database seems to me like it might cause some deferred risks. Specifically, the user might simply have misplaced or misnamed the DB they actually wanted to use, and wind up doing multiple downloads and having multiple databases with conflicting statuses, and we're going to get confused questions about what they need to do now. I feel like, since we already need to share accounts.json with a GUI instance, we might as well just also share the DB with them. On the other hand, the other settings are a real pain to share with a GUI. The GUI has some clever logic that does code point replacements for invalid Windows characters, but it only does that when it creates a new config file, and only when it starts on Windows. I want those replacements on my Docker, because that Docker is creating files I'm going to use on Windows (and Linux, and Android). But I also want the paths to keep working for Docker, because of course the same file with those settings also contains paths that are specific to the Windows installation that created them. I guess what I'm saying is, maybe we could process the settings files to make them appropriate for Docker. |
|
I was a bit concerned about creating the blank db for that reason. I would really like to make initial setup of the docker image as easy as possible. One easy thing I can do is check for any For the character replacement, I'd rather not hard code those values into the docker script. It would be best if Libation could generate the defaults on demand and we can either use it as the default settings file or I can merge that section with the provided settings. |
|
I've reworked the image startup script to do the following regarding database files:
This isn't fool proof, but it adds a decent amount of guard rails around database loading while trying to maximize how easy it is to adopt. If we start seeing an influx of people having problems with the database, I'm happy to revisit these. I've also had my modified image (pre-db-rework) running at home for the last few weeks without problem/ |
|
Sounds good. I assume since the PR is still in draft that you intend to do more before I merge. |
|
I'd like it if someone was willing to try the various envvar combinations in case I missed something with my testing. I'll move it out of draft and leave it up to you on how tested is enough tested. |
|
@rmcrackan When you create the release, can you put a warning section in the notes? You can copy this one: Warning This release contains breaking changes for the docker image. Read Docker.md before upgrading. |
|
I don't have docker testers per se. It's whoever volunteers. I'll merge this and create a pre-release with your warning. Nice warning markdown btw -- I've not seen that before. |
|
It looks like the docker release no longer works after this PR: https://github.com/rmcrackan/Libation/actions/runs/11844958812/job/33009709209 |
|
I'll get a fix for it shortly. |
|
Hey docker folks, can you take a look at this comment about these recent changes? #1049 (comment) I'm not familiar with how docker uses |
Fixes #439
Warning
This PR contains breaking changes for the docker image. Read Docker.md before upgrading.
This contains a pretty big overhaul of the docker image. The main changes are:
1001and group1001by default. It also supports having that overridden by docker at runtime.Settings.jsonno longer needs to be edited by hand to specify the correct file paths. The image will substitute the correct values as runtime.Together, these should make it easier for a new user to get the image up and running. At a minimum now, they only need to provide the AccountsSettings.json and it will work.
@wtanksleyjr @robflate @muchtall - Are any of you willing to review this PR and test it before it's merged?
Testing
I tested the following scenarios:
BooksandInProgress