Skip to content

Add Dockerfile #128

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.

Sign up for GitHub

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
gesellix wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add Dockerfile #128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .dockerignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
dist/
Copy link

@SuperSandro2000 SuperSandro2000 Feb 5, 2020

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

@gesellix gesellix Feb 5, 2020

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

testpages/*
tests/test_results.txt
robots.html
24 changes: 24 additions & 0 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
FROM alpine:3.10 AS builder
Copy link

@SuperSandro2000 SuperSandro2000 Feb 5, 2020

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

LABEL builder=true

ENV CGO_ENABLED=0
Copy link

@SuperSandro2000 SuperSandro2000 Feb 5, 2020

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

@gesellix gesellix Feb 5, 2020

Choose a reason for hiding this comment

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

Do we need this?

Copy link

@SuperSandro2000 SuperSandro2000 Feb 5, 2020

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 GOPATH /go

RUN apk add --update -t build-deps go git mercurial libc-dev gcc libgcc
Copy link

@SuperSandro2000 SuperSandro2000 Feb 5, 2020 •
edited
Loading

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.

RUN go get github.com/ericchiang/pup \
&& cd $GOPATH/src/github.com/ericchiang/pup \
&& go build \
-a \
-ldflags '-s -w -extldflags "-static"' \
-o /bin/pup
RUN adduser -DH user
Copy link

@SuperSandro2000 SuperSandro2000 Feb 5, 2020

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

@gesellix gesellix Feb 5, 2020

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

@SuperSandro2000 SuperSandro2000 Feb 5, 2020

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

@gesellix gesellix Feb 5, 2020

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

CMD [ "--help" ]

COPY --from=builder /etc/passwd /etc/passwd
Copy link

@SuperSandro2000 SuperSandro2000 Feb 5, 2020

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

@gesellix gesellix Feb 5, 2020

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.

Copy link

@SuperSandro2000 SuperSandro2000 Feb 5, 2020

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

@gesellix gesellix Feb 5, 2020

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

Copy link

@SuperSandro2000 SuperSandro2000 Feb 5, 2020

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

@gesellix gesellix Feb 5, 2020

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 😉

USER user

COPY --from=builder /bin/pup /pup
11 changes: 9 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,21 @@ fast and flexible way of exploring HTML from the terminal.

Direct downloads are available through the [releases page](https://github.com/EricChiang/pup/releases/latest).

If you have Go installed on your computer just run `go get`.
If you have Go installed on your computer just run `go get`:

go get github.com/ericchiang/pup

If you're on OS X, use [Homebrew](http://brew.sh/) to install (no Go required).
If you're on OS X, use [Homebrew](http://brew.sh/) to install (no Go required):

brew install https://raw.githubusercontent.com/EricChiang/pup/master/pup.rb

If you have Docker installed, use the Docker image:

docker run --rm pup --help

# an alias makes it even shorter
alias pup="docker run --rm pup"

## Quick start

```bash
Expand Down