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

use golang:alpine for golang docker image #194

Merged
merged 2 commits into from Jan 3, 2018

Conversation

vladbarosan
Copy link

@vladbarosan vladbarosan commented Dec 19, 2017

Address #176

  • Use golang:alpine since golang:onbuild is deprecated and discouraged from being used.
  • Use multi-stage build
  • use ca-certificates to enable SSL certificate check

@vladbarosan
Copy link
Author

@brettcannon can you have a look please, this addresses issue #176

@brettcannon
Copy link
Member

@vladbarosan I'm definitely not qualified to review this since I don't code in Go. 😁

FROM golang:alpine
WORKDIR /go/src/app
COPY . .
RUN apk update && apk upgrade && apk add --no-cache bash git openssh
Copy link

Choose a reason for hiding this comment

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

not all would agree with including SSH, see e.g. link. my vote would be to leave it out.

Choose a reason for hiding this comment

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

There's no reason to include ssh in a generic builder.

@joshgav
Copy link

joshgav commented Dec 19, 2017

thanks @vladbarosan! would it be better to provide a multi-stage build? see e.g. link. Buffalo creates a multistage build too, see https://github.com/gobuffalo/buffalo/blob/master/generators/docker/multi/templates/Dockerfile.tmpl

@joshgav
Copy link

joshgav commented Dec 19, 2017

@bketelsen
Copy link

Go containers are often serving TLS services, it would make sense to add ca-certificates to this dockerfile so that certificates can be verified.

@vladbarosan
Copy link
Author

@joshgav pushed an update using multi-stage build. @bketelsen I added ca-certificates. Let me know if there is anything we need here or we are good to go.

@bketelsen
Copy link

Lgtm

@vladbarosan
Copy link
Author

@brettcannon can we merge this please ? thanks

@brettcannon
Copy link
Member

@vladbarosan I'm still the wrong person to ask this as I've never written a Dockerfile and I still don't code in Go.

Copy link

@joshgav joshgav left a comment

Choose a reason for hiding this comment

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

LGTM modulo my one comment.

@chrisdias can you review and merge? thanks!

FROM alpine:latest
RUN apk --no-cache add ca-certificates
COPY --from=builder /go/bin/app /app
ENTRYPOINT ./app
Copy link

Choose a reason for hiding this comment

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

should this be /app?

@chrisdias chrisdias merged commit 13de5c0 into microsoft:master Jan 3, 2018
@chrisdias
Copy link
Member

LGTM

@microsoft microsoft locked and limited conversation to collaborators Oct 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants