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

Multistage Dockerfiles for linux and windows #44

Merged
merged 3 commits into from
Feb 16, 2018
Merged

Conversation

ColinSullivan1
Copy link
Member

Add multistage dockerfiles for:

  • linux/amd64
  • linux/arm32v7
  • linux/arm64v8
  • windows/nanoserver
  • windows/windowsservercore

After a release has been cut, the git clone APIs need to reference a release tag.

@coveralls
Copy link

coveralls commented Feb 15, 2018

Coverage Status

Coverage remained the same at 90.508% when pulling ceda3e5 on dockerfiles into 9c93edd on master.

Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

Please check link about ENTRYPOINT to see if it applies. Otherwise, except for a missing GOOS=linux (which probably is not required), LGTM.

RUN git clone https://github.com/nats-io/prometheus-nats-exporter.git .

# build
RUN CGO_ENABLED=0 GOARCH=arm GOARM=7 go build -v -a -tags netgo -installsuffix netgo -ldflags "-s -w"
Copy link
Member

Choose a reason for hiding this comment

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

I guess it is default, but should there be GOOS=linux here?

FROM microsoft/nanoserver
COPY --from=0 /go/src/github.com/nats-io/prometheus-nats-exporter/prometheus-nats-exporter.exe /prometheus-nats-exporter.exe
EXPOSE 7777
ENTRYPOINT ["/prometheus-nats-exporter.exe"]
Copy link
Member

Choose a reason for hiding this comment

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

I have been told that there should not be ENTRYPOINT here (same for windowsservercore dockerfile). Please check nats-io/nats-streaming-docker@9596dfa#diff-e2b7dae7d59ab0b01b6fa33dfcc4c7af for details.

Copy link
Member

Choose a reason for hiding this comment

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

We need to update this in gnatsd too by the way. I did not get to do it yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. I followed the chain of links and comments that finally lead here - because one can powershell into the images, we need to use CMD. I'll change this, but as we discussed offline, imo this isn't actually what we want in our images. If there were such a thing as a scratch that ran with windows, I'd use it here.

@ColinSullivan1
Copy link
Member Author

FYI, updated based on comments.

@ColinSullivan1 ColinSullivan1 merged commit cce7e14 into master Feb 16, 2018
@ColinSullivan1 ColinSullivan1 deleted the dockerfiles branch February 16, 2018 23:39
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