updated docker file #22
Conversation
macwinnie
commented
Apr 20, 2020
macwinnie
commented
- install latest version with all dependencies
- remove unwanted additional image layers
- expose ports 53 and 8080 so they can easily be adapted
|
Thanks! Does this ignore the host dns server?
…On Tue, 21 Apr 2020 at 8:38 am Martin Winter ***@***.***> wrote:
- install latest version with all dependencies
- remove unwanted additional image layers
- expose ports 53 and 8080 so they can easily be adapted
------------------------------
You can view, comment on, or merge this pull request online at:
#22
Commit Summary
- updated docker file
File Changes
- *M* Dockerfile
<https://github.com/jpillora/docker-dnsmasq/pull/22/files#diff-3254677a7917c6c01f55212f86c57fbf>
(32)
Patch Links:
- https://github.com/jpillora/docker-dnsmasq/pull/22.patch
- https://github.com/jpillora/docker-dnsmasq/pull/22.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#22>, or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE2X432EMAP7HYHFJTNS4DRNTFFXANCNFSM4MM2J6VQ>
.
|
|
What do you mean by "ignore the host dns server"? Do you think about issue #21 ? |
| #configure dnsmasq | ||
| RUN mkdir -p /etc/default/ | ||
| RUN echo -e "ENABLED=1\nIGNORE_RESOLVCONF=yes" > /etc/default/dnsmasq |
There was a problem hiding this comment.
do we not need this file anymore?
There was a problem hiding this comment.
@jpillora this is in response to your review comments from earlier, github UI may not make that obvious.
They moved these two RUN lines into the first RUN to keep them all in the same layer.
They droped the two ENV declarations at the start of the file to call some shell script file you're hosting at https://i.jpillora.com/webproc, piping that through bash to retrieve the latest version of webproc that way. While nice, it will remove immutability of the build as the version is no longer pinned.
That script does some of the logic here, unarchiving the file and outputting a binary with execution rights enabled via chmod +x, it just needs to move the binary.
The ENTRYPOINT change is just whitespace formatting and renaming the --config option to match the newer option name you must have modified while developing webproc.
Addition of EXPOSE only serves value of communicating what ports the user should map to the image, it doesn't really do any thing beyond that.
There was a problem hiding this comment.
Ah yes, they just moved it 👍
Agreed, probably safer to keep existing logic and use the direct github URL.
|
Is it commited right now? |
There was a problem hiding this comment.
LGTM, I would advise alpine:3.12 instead of alpine:latest, although this PR prevents immutable builds due to the change with curl no longer using a versioned URL.
If you're ok with that concern, then the PR is fine, trigger a new build of the same Dockerfile without any updates to it when you want to push a new release.
| #configure dnsmasq | ||
| RUN mkdir -p /etc/default/ | ||
| RUN echo -e "ENABLED=1\nIGNORE_RESOLVCONF=yes" > /etc/default/dnsmasq |
There was a problem hiding this comment.
@jpillora this is in response to your review comments from earlier, github UI may not make that obvious.
They moved these two RUN lines into the first RUN to keep them all in the same layer.
They droped the two ENV declarations at the start of the file to call some shell script file you're hosting at https://i.jpillora.com/webproc, piping that through bash to retrieve the latest version of webproc that way. While nice, it will remove immutability of the build as the version is no longer pinned.
That script does some of the logic here, unarchiving the file and outputting a binary with execution rights enabled via chmod +x, it just needs to move the binary.
The ENTRYPOINT change is just whitespace formatting and renaming the --config option to match the newer option name you must have modified while developing webproc.
Addition of EXPOSE only serves value of communicating what ports the user should map to the image, it doesn't really do any thing beyond that.
| @@ -1,18 +1,24 @@ | |||
| FROM alpine:edge | |||
| FROM alpine:latest | |||
There was a problem hiding this comment.
Note that this change will switch from edge to the latest supported Alpine release (currently 3.12). It would be wiser to be more explicit and just state 3.12 here and update accordingly in the future when newer releases of Alpine or dnsmasq are made available.
There was a problem hiding this comment.
Agreed, should be explicit version
|
@FlyingEagle your PR is fine 👍 The only problem would be reproducible builds over time if required. |
|
See upgrade discussion here #24 |
