Skip to content

fix(Dockerfile): readability and golang image update #45

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

Merged
merged 10 commits into from
Jul 26, 2022
Merged

fix(Dockerfile): readability and golang image update #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e912938
fix(Dockerfile): readability and golang image update
Jul 7, 2022
f126524
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 7, 2022
6daad1b
fix(Dockerfile): addressing comments and adding dependabot for docker
mridulganga Jul 8, 2022
e3287b4
Addressing comments
mridulganga Jul 11, 2022
38560e9
Merge branch 'main' into dockerfile-fix
shukra-in-spirit Jul 12, 2022
5e87c80
Merge branch 'main' into dockerfile-fix
Jul 18, 2022
452aa49
refactor(Dockerfile): Remove redundant comment
Jul 26, 2022
84afbb1
refactor(Dockerfile): Fix comment
Jul 26, 2022
ed33a5b
Merge branch 'v0.2.0-review-updates' into dockerfile-fix
haani-niyaz Jul 26, 2022
4d9a96a
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Jul 26, 2022
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 .github/dependabot.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,7 @@ updates:
schedule:
interval: weekly
open-pull-requests-limit: 5
- package-ecosystem: "docker"
directory: "/"
schedule:
interval: "weekly"
4 changes: 4 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,9 @@ bin/
# Dependency directories (remove the comment below to include it)
# vendor/

# Code editor personal settings
.vscode/
Copy link

Choose a reason for hiding this comment

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

Would it be beneficial to still keep task, launch, extensions etc? Personally have used this as a reference in the past: https://github.com/github/gitignore/blob/main/Global/VisualStudioCode.gitignore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we should not impose anyone's preferences of IDE config on others I think.

Copy link
Member

Choose a reason for hiding this comment

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

Forgive my persistence here, but why is this needed @mridulganga?

I think its quite a given that one should not include the .vscode folder here since that is used for local development.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since .vscode is used for local dev, we should not checkin that folder, so I included the same in .gitignore.

.idea/

# Other
.DS_Store
23 changes: 16 additions & 7 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,22 @@
FROM golang:1.18 as build
WORKDIR /go/src/app
FROM golang:1.18-alpine as builder

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

WORKDIR /src
COPY . .
# Static build requires CGO_ENABLED=0
RUN mkdir -p /go/bin && CGO_ENABLED=0 go build -ldflags="-w -s" -o /go/bin/app ./...

# Build executable
RUN make build

# Using a distroless image from https://github.com/GoogleContainerTools/distroless
# Image sourced from https://console.cloud.google.com/gcr/images/distroless/global/static
FROM gcr.io/distroless/static:nonroot
COPY --from=build /go/bin/app /
# numeric version of user nonroot:nonroot provided in image

# Copy executable from builder image
COPY --from=builder /src/bin/app /

# Numeric version of user nonroot:nonroot provided in image
USER 65532:65532

# Run the executable
CMD ["/app"]
4 changes: 3 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
SHELL=/bin/bash -e -o pipefail
Copy link
Collaborator

Choose a reason for hiding this comment

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

@mridulganga, Not sure if you tested this but this line would cause an error since /bin/bash doesn't exist in alpine. To make this work and for the sake of portability we should use the default shell /bin/sh so I would suggest removing this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added bash now as well

PWD = $(shell pwd)
GO_BUILD= go build
GOFLAGS= CGO_ENABLED=0

## help: Print this help message
.PHONY: help
Expand Down Expand Up @@ -37,4 +39,4 @@ fmt:
## build: Build binary into bin/ directory
.PHONY: build
build:
go build -ldflags="-w -s" -o bin/app ./...
$(GOFLAGS) $(GO_BUILD) -a -v -ldflags="-w -s" -o bin/app cmd/main.go
Copy link
Collaborator

Choose a reason for hiding this comment

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

This won't cross compile since the GOOS value is linux so for instance on my mac it would fail. This was one of the reasons the Dockerfile and Makefile commands were different. The original Makefile command was designed for local testing.

CGO_ENABLED=0 was missing so we definitely need that one 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the GOOS variable

Copy link

Choose a reason for hiding this comment

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

do we need to also think about injecting a global variable Version during linking?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jeffy-mathew yup, it is useful to have but perhaps as a separate issue. Feel free to create one.

3 changes: 3 additions & 0 deletions sonar-project.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,6 @@ sonar.organization=rog-golang-buddies

# Encoding of the source code. Default is default system encoding
#sonar.sourceEncoding=UTF-8

# ensure that test files are identified and not used to check coverage
sonar.test.inclusions=**/*_test.go
Copy link

Choose a reason for hiding this comment

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

If we decide to remove this file, we could add an argument in gh sonarcloud workflow:
-Dsonar.test.inclusions=**/*_test.go