Skip to content

Improvements to Dockerfile #42

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

Closed
mridulganga opened this issue Jul 7, 2022 · 6 comments · Fixed by #45
Closed

Improvements to Dockerfile #42

mridulganga opened this issue Jul 7, 2022 · 6 comments · Fixed by #45
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@mridulganga
Copy link
Contributor

mridulganga commented Jul 7, 2022

The existing Dockerfile is good for a starter project, but for a template repo we should follow some best practices.
I am including the below code for reference.

FROM golang:1.18 as builder

RUN apk update && apk upgrade && \
    apk add --no-cache make

WORKDIR /src
COPY . .

# get packages and build
RUN make tidy
RUN make build

# Using a distroless image from https://github.com/GoogleContainerTools/distroless
FROM gcr.io/distroless/static:nonroot

COPY --from=build /app/bin/app /

USER 65532:65532
CMD["/app"]

ENV PORT=8080
EXPOSE $PORT
@mridulganga mridulganga added enhancement New feature or request good first issue Good for newcomers labels Jul 7, 2022
@mridulganga
Copy link
Contributor Author

Additionally we should also consider making use of golangci-lint
which will allow us to add multiple linting/vet/fmt tools in its configuration.

@haani-niyaz
Copy link
Collaborator

@mridulganga thanks for raising this! A few thoughts...

RUN make tidy

I wouldn't do this inside the builder. To reference the docs:

go mod tidy ensures that the go.mod file matches the source code in the module. It adds any missing module requirements necessary to build the current module’s packages and dependencies, and it removes requirements on modules that don’t provide any relevant packages. It also adds any missing entries to go.sum and removes unnecessary entries.

This means, you'll have a working build however it does not represent the state of your go.mod and go.sum file in your repo, as it is dynamically fixed. IMO, it is better to have the build fail because of outdated module files than to have it transparently fixed, with the fixed state not represented in your source code.

ENV PORT=8080
EXPOSE $PORT

I originally debated this one because you may have outbound-only services that are not meant to be exposed e.g: queue worker cli etc. From a template generation perspective, it would make sense to opt-in for this though.

@haani-niyaz
Copy link
Collaborator

Additionally we should also consider making use of golangci-lint which will allow us to add multiple linting/vet/fmt tools in its configuration.

We already have golangci-lint as a github workflow and in the pre-commit hook setup.

@mridulganga
Copy link
Contributor Author

FROM golang:1.18.3-alpine3.16 as builder

RUN apk update && apk upgrade && \
    apk add --no-cache make

WORKDIR /src
COPY . .

# build executable
RUN make build

# Using a distroless image from https://github.com/GoogleContainerTools/distroless
FROM gcr.io/distroless/static:nonroot

COPY --from=build /app/bin/app /

USER 65532:65532
CMD["/app"]

ENV PORT=8080
EXPOSE $PORT

@haani-niyaz I have updated it as you suggested, additionally I also added golang:1.18.3-alpine3.16 image as the 1.18 golang image is massive 2GB+

I was not aware of golangci-lint as it wasn't mentioned in readme, perhaps we can plan and add it there too.

@haani-niyaz
Copy link
Collaborator

@mridulganga good pickup on the image size 👍. Feel free to raise a PR to fix the image tag.

I was not aware of golangci-lint as it wasn't mentioned in readme, perhaps we can plan and add it there too.

Yeah the README needs an updated. It is grossly outdated.

@mridulganga
Copy link
Contributor Author

@rog-golang-buddies/platform-team @haani-niyaz
I have created a PR with the changes we discussed in this issue and some additional minor changes (more info in PR description)
#45

@mridulganga mridulganga linked a pull request Jul 7, 2022 that will close this issue
@mridulganga mridulganga self-assigned this Jul 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants