Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add Dockerfile #128

wants to merge 1 commit into from

Conversation

gesellix
Copy link

@gesellix gesellix commented Feb 3, 2020

Adds a Dockerfile, which you might use in automated builds on the Dockerhub.

Relates to #126

@@ -0,0 +1,4 @@
dist/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are missing .git

Copy link
Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ENV CGO_ENABLED=0
ENV CGO_ENABLED=0 GOPATH=/go

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this?

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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line is useless

Copy link
Author

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" ]

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?

Copy link
Author

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

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.

Copy link
Author

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.

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.

Copy link
Author

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).

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.

Copy link
Author

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 😉

@SuperSandro2000
Copy link

Also when I try to build this I get:

/pup # go get github.com/ericchiang/pup
# github.com/ericchiang/pup
loadinternal: cannot find runtime/cgo

I would suggest using golang:alpine as a base image as this is the only working workaround I could find for that issue.

@SuperSandro2000
Copy link

SuperSandro2000 commented Feb 5, 2020

I quickly wrote this Dockerfile which in my opinion is way nicer, smaller and easier to maintain.

FROM golang:alpine as builder

RUN apk add --no-cache git

RUN go get github.com/ericchiang/pup

WORKDIR $GOPATH/src/github.com/ericchiang/pup

RUN go build -a -ldflags '-s -w -extldflags "-static"' -o /bin/pup

#----------#

FROM scratch

COPY --from=builder /bin/pup /pup

ENTRYPOINT [ "/pup" ]
CMD [ "--help" ]

btw the user switch is not needed as the resulting container is basically empty except pup.

@matthewadams
Copy link

Can y'all get this merged and building automatically on hub.docker.com please?

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.

None yet

3 participants