Skip to content

Rootless + smaller image + cleanup #402

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

Open
wants to merge 15 commits into
base: v1.10.0
Choose a base branch
from

Conversation

chevdor
Copy link

@chevdor chevdor commented Nov 19, 2024

This PR looks bigger than it is, due to the removal of leading spaces...

Here are the changes:

  • all the doc moves to it own subfolder
  • the scripts also gets a cosier home
  • container files (like Dockerfile) move to a new container subfolder
  • the container image (even though it was not the goal) shrinks from 582MB to 405MB
  • the container is now rootless 🎉
  • addition of a justfile, it is convenient for those using it, inert for others. It provides also a nice doc and shows how to build the images. See below.

justfile

Users not using just can just ignore.
For others, you need to set your IMAGE=... in the .env file. It will typically look like IMAGE=your_name/lndg.
Alternatively, you may provide your own registry: IMAGE=docker.io/chevdor/lndg.
After that, you may just 😄 run: just build_image and just push_image.

WARNING: Change for Docker users lndg is now in /lndg/ and no longer /app.

@chevdor chevdor marked this pull request as ready for review November 19, 2024 10:44
@chevdor chevdor marked this pull request as draft November 19, 2024 10:55
@chevdor chevdor marked this pull request as ready for review November 19, 2024 11:12
@cryptosharks131 cryptosharks131 changed the base branch from master to v1.10.0 November 24, 2024 13:48
@gnzsnz
Copy link

gnzsnz commented Mar 4, 2025

I'm glad i found this PR. I was actually going to do something similar. Although my plan was to change only Dockerfile, docker-compose.yml and create an entry-point.sh

if it's not too late, i would like to suggest a few things.

  • I'm not familiar with justi think it's an interesting concept. if i understand the justfile correctly, we have shortcuts for building the image and pushing it to dockerhub. I would suggest achieving the same through existing tools, which will avoid the addition of a new tool. docker-compose.yml compose can handle the build part elegantly, once the build is done, push is just a simple docker compose push. I can provide docker-compose.yml that are needed.
  • i would like to propose to use github actions to publish docker images. i can provide the actions yml file. with this approach, an image is build in github after a release tag is pushed to github. Which basically automates the build and push process. i can provide these changes too.
  • parametrize Dockerfile user name, uid 1000 is a great default value. but we can parametrize it and include it in the docker-compose.yml which simplifies creating new images with a custom uid. Which is great for security.

Please let me know if you agree with my proposal, so i can prepare the changes

@chevdor
Copy link
Author

chevdor commented Mar 5, 2025

As I mentioned in the comment, just is not mandatory. It makes it easier for those using it and it is a good "documentation" otherwise. Can it be replaced by other tools ? Sure.

The initial goal of my PR was not a fully fledge auto-build but improving the image, its size, security and the organization of the scripts.

I agree however that, once this PR is merged, the build should be automated. Bigger PRs are harder to review and get merged. It seems that my PR is "big" enough so I would suggest lining up a new PR for the automation and wait until this one gets merged.

i would like to propose to use github actions to publish docker images

100% with you. Just beware that there was no reaction on my PR in a while, I am not sure that it is considered as "interesting" despite the security increase....

parametrize Dockerfile user name, uid 1000

That would be a good addition indeed.

@cryptosharks131
Copy link
Owner

Thanks for the PR and I support making some updates to the Docker setup here.

However, since the PR is quite large, I think it would be good to move some of these updates into separate PRs to be merged.

@chevdor
Copy link
Author

chevdor commented Apr 1, 2025

I tried keeping it small but most of the refactoring are motivated by the rework of the image.
How would you split ?

@cryptosharks131
Copy link
Owner

I think it would be good to focus on just whats needed for the rootless Dockerfile updates with a smaller image. It is a bit clouded by all the cleanup items when trying to review.

@chevdor
Copy link
Author

chevdor commented Apr 7, 2025

I know the discomfort of reviewing big PRs :)

If you fly over the formatting and file moving for html and md files (whose changes are insignificant to very small), there is not much left really.

I could make an extra PR to get those missing EOF/EOL out of this PR but this is quite some effort for a small gain.

Moving files around was mainly motivated to keeping filters and copies simpler so it is hard to remove those without making the PR actually more complex.

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.

4 participants