-
Notifications
You must be signed in to change notification settings - Fork 96
Add Docker pipeline #295
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
base: master
Are you sure you want to change the base?
Add Docker pipeline #295
Conversation
mh166
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't review the GitHub actions part as I'm not familiar with it. At least, it looks plausible. 😅 However, I want to recommend some minor tweaks to the Dockerfile to minimize the resulting image layers.
The same optimization could, in theory, be made for the build image. However, since this is just an ephemeral image, it really doesn't matter and therefore I left it out.
Dockerfile
Outdated
|
|
||
|
|
||
| # Clean-up dist | ||
| RUN rm -rf /dist |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, this should be combined into one RUN command instead to reduce overhead:
RUN make install && rm -rf /distThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Edit: Sorry for the spam. 🙈 I messed up the previous review... This is the same issue as mentioned above...
Dockerfile
Outdated
|
|
||
| ## Install shared dependencies | ||
| RUN apk update | ||
| RUN apk add --no-cache openssl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is good practice to combine the RUN commands into on single command in order to not create unnecessary layers in the final images. As the shared image is also being used by the final image, it would make sense to combine them:
RUN apk add --no-cache openssl make lua5.1 pcre2Also, apk update is not needed here: when called with the --no-cache option, apk add does not make use of any locally cached information (as provided by apk update) but instead fetches all required data from the repo. 😊
|
I would like to avoid maintaining a docker setup inside the codebase. Instead such contributions can go in the Wiki. And actually there's already a simple Docker setup page there: https://github.com/lefcha/imapfilter/wiki/Docker Feel free to modify this page or add a new one with a more advanced/complicated setup if you like. Also note that there's another similar open PR: #300 |
|
@lefcha Hello! I've reworked Wiki-page for Docker support as you suggested, you can view it here. I'm open to suggestions and feel free to "pull" my version. Unfortunately, GitHub does not support the same fork-pull-merge workflow for Wikis, so you can just copy-paste my version. I've also integrated @mh166 suggestions. I will commit current version from my Wiki-page to this branch now for convenience. |
|
Or, we can create a separate repo just for container image pipeline like other projects do. And push to docker hub, GHCR or whatever from there. |
Add proper1 pipeline to build and push a lightweight (based on Alpine) Docker image with imapfilter; I expect it to replase this drastically outdated image at DockerHub.
1 — free and open, with easily verifiable source code.