-
Notifications
You must be signed in to change notification settings - Fork 256
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
Add Dockerfile #128
base: master
Are you sure you want to change the base?
Add Dockerfile #128
Conversation
@@ -0,0 +1,4 @@ | |||
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.
You are missing .git
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.
Right. It doesn't have any impact on the final image, though. I'll add a fixup later
@@ -0,0 +1,24 @@ | |||
FROM alpine:3.10 AS builder |
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.
FROM alpine:3.10 AS builder | |
FROM alpine:3.11 AS builder |
FROM alpine:3.10 AS builder | ||
LABEL builder=true | ||
|
||
ENV CGO_ENABLED=0 |
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.
ENV CGO_ENABLED=0 | |
ENV CGO_ENABLED=0 GOPATH=/go |
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.
Do we need this?
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 looks nicer and is easier to convert it to an export ...
.
ENV CGO_ENABLED=0 | ||
ENV GOPATH /go | ||
|
||
RUN apk add --update -t build-deps go git mercurial libc-dev gcc libgcc |
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.
RUN apk add --update -t build-deps go git mercurial libc-dev gcc libgcc | |
RUN apk add --no-cache -t build-deps go git mercurial libc-dev gcc |
You don't update base image packages unless it is needed for a reason.
The removed packages are already pulled in with go.
-a \ | ||
-ldflags '-s -w -extldflags "-static"' \ | ||
-o /bin/pup | ||
RUN adduser -DH user |
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.
this line is useless
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's preparation for the next build step
|
||
FROM scratch | ||
|
||
ENTRYPOINT [ "/pup" ] |
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.
Did it get +x
somewhere?
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.
go build
handles that one
ENTRYPOINT [ "/pup" ] | ||
CMD [ "--help" ] | ||
|
||
COPY --from=builder /etc/passwd /etc/passwd |
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.
not needed, as you create the user again and the copy can be chowned if needed.
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.
See above, the user is only created in another build step, here we only apply the minimal change to "create" the user. The user itself is not really necessary, but I consider it a best practice not to run everything as root
.
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.
If we use FROM scratch
we can safely use the root user. There is nothing we can escape to.
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.
"best practice" refers more to "give future users a chance to make things right by default", so yes, FROM scratch
minimizes the attack surface, but what happens if anybody wants to convert the image to a Windows container where we don't have an empty base image? Not that I'd do that, but I prefer to keep those minimal baselines and make people become aware of those aspects.
In fact, I don't actually care if the maintainer of pup
thinks the same way, so I consider this PR as simple proposal which can have alternate solutions with more focus on simplicity (see your own Dockerfile you already mentioned).
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 am pretty sure that a windows variant would have at least 10 more problems cause it is windows and you can't consider everything you might maybe need in the future.
Also this would be a problem for the windows eco system when they only support half of docker.
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.
Yes, you're right and I fully agree. I hope you got my point, though 😉
Also when I try to build this I get:
I would suggest using |
I quickly wrote this Dockerfile which in my opinion is way nicer, smaller and easier to maintain.
btw the user switch is not needed as the resulting container is basically empty except pup. |
Can y'all get this merged and building automatically on hub.docker.com please? |
Adds a Dockerfile, which you might use in automated builds on the Dockerhub.
Relates to #126