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

update Dockerfile to run as non-root #83

Closed
wants to merge 9 commits into from

Conversation

guusvw
Copy link

@guusvw guusvw commented Oct 6, 2019

solves #82
Signed-off-by: Guus van Weelden guus.vanweelden@moia.io

Signed-off-by: Guus van Weelden <guus.vanweelden@moia.io>
.dockerignore Outdated
@@ -1,2 +1,3 @@
.git
.travis.yml
Copy link

Choose a reason for hiding this comment

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

I think we still want this

Copy link
Author

Choose a reason for hiding this comment

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

removed it. it was part of the .dockerignore, but fine for me ;)

@digikin
Copy link
Contributor

digikin commented Oct 7, 2019

There is no reason to add all that group and env into the file. You are only copying a binary into the folder. Have you looked or ran my dockerfile?

@guusvw guusvw requested a review from hbagdi October 7, 2019 18:03
Comment on lines +14 to +17
ARG USER=deck
ARG GROUP=deck
ARG UID=9999
ARG GID=9999
Copy link
Author

Choose a reason for hiding this comment

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

This allows you to set those variables from outside during the build time of the container @digikin

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we really had to template these in the first place. This is a self-contained and small Dockerfile.

No strong feelings here so not changes necessary.

RUN apk --no-cache add ca-certificates
WORKDIR /root/
RUN apk update \
&& apk upgrade \
Copy link
Member

Choose a reason for hiding this comment

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

Why is an upgrade necessary here?

@hbagdi
Copy link
Member

hbagdi commented Oct 11, 2019

@guusvw Friendly ping.

@hbagdi
Copy link
Member

hbagdi commented Oct 20, 2019

#84 implements this.
Thank you for your PR!

@hbagdi hbagdi closed this Oct 20, 2019
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.

4 participants