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

mridulganga · 2022-07-07T19:24:49Z

Summary of changes

Made changes to Dockerfile to use golang's alpine image for smaller size which helps build image faster
Added make build to build the golang binary replacing the embedded build command
Added an Optional PORT env var and expose command
Minor changes to Makefile to use specific main.go file instead of folder, and added few build args (-v - listing packages while building and -a - rebuilding prebuilt packages everytime) and envs (to disable cgo and setting GOOS)
Minor change in sonar-project.properties to add test inclusions parameter.
Minor change in .gitignore to ignore vs code and pycharm IDE config folders.

for more information, see https://pre-commit.ci

haani-niyaz

Thanks for the PR. I've added some review comment.

haani-niyaz · 2022-07-07T22:23:29Z

Dockerfile

@@ -1,13 +1,27 @@
-FROM golang:1.18 as build
-WORKDIR /go/src/app
+FROM golang:1.18.3-alpine3.16 as builder


Perhaps we should set this to golang:1.18-alpine so that it always gets the latest changes? This way it would be one less thing for teams to update.

addressed and updated this.

haani-niyaz · 2022-07-07T22:27:02Z

Makefile

@@ -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


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 👍

Removed the GOOS variable

haani-niyaz · 2022-07-07T22:33:05Z

Dockerfile

+
+# optionally expose a port
+ENV PORT=8000
+EXPOSE $PORT


I'm on the fence about this one for the reasons covered earlier. To reiterate not all projects will expose a service e.g: queue workers, CLIs etc. I'm of the opinion we should enable teams to add things and not have them worry about removing things.

Removed port

jeffy-mathew · 2022-07-08T04:48:49Z

Makefile

@@ -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


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

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

M1kep · 2022-07-08T23:06:55Z

.gitignore

@@ -15,3 +15,7 @@
 # vendor/

 bin/
+
+# code editor personal settings
+.vscode/


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

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

haani-niyaz · 2022-07-10T02:01:21Z

Dockerfile

-COPY --from=build /go/bin/app /
+
+# copy executable from builder image to final images
+COPY --from=build /src/bin/app /


@mridulganga this needs to be updated to the image built from the previous stage. You renamed it to builder but it is referenced as build here. Was this tested locally?

Done this change now.

haani-niyaz · 2022-07-10T02:06:04Z

Dockerfile

 FROM gcr.io/distroless/static:nonroot
-COPY --from=build /go/bin/app /
+
+# copy executable from builder image to final images


@mridulganga Can we follow the comment convention of using uppercase first letter, thanks.

Also I think it should say "Copy executable from builder image"

Updated comments to have first letter caps

@mridulganga I don't know what you mean by "final images". The comment should be updated as I pointed out earlier.

haani-niyaz · 2022-07-10T05:55:57Z

Makefile

@@ -1,5 +1,7 @@
 SHELL=/bin/bash -e -o pipefail


@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.

Added bash now as well

pallasite99 · 2022-07-13T15:26:18Z

.gitignore

@@ -15,3 +15,7 @@
 # vendor/

 bin/
+
+# code editor personal settings
+.vscode/


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.

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

ghost · 2022-07-17T15:34:55Z

sonar-project.properties

@@ -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


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

for more information, see https://pre-commit.ci

haani-niyaz · 2022-07-26T08:41:59Z

Merging changes to v0.2.0 branch to control next release.

mridulganga and others added 2 commits July 8, 2022 00:54

fix(Dockerfile): readability and golang image update

e912938

[pre-commit.ci] auto fixes from pre-commit.com hooks

f126524

for more information, see https://pre-commit.ci

mridulganga mentioned this pull request Jul 7, 2022

Improvements to Dockerfile #42

Closed

mridulganga linked an issue Jul 7, 2022 that may be closed by this pull request

Improvements to Dockerfile #42

Closed

haani-niyaz requested changes Jul 7, 2022

View reviewed changes

jeffy-mathew reviewed Jul 8, 2022

View reviewed changes

fix(Dockerfile): addressing comments and adding dependabot for docker

6daad1b

mridulganga requested a review from haani-niyaz July 8, 2022 18:48

M1kep reviewed Jul 8, 2022

View reviewed changes

haani-niyaz reviewed Jul 10, 2022

View reviewed changes

Addressing comments

e3287b4

mridulganga mentioned this pull request Jul 11, 2022

.vscode added into .gitignore #51

Closed

Merge branch 'main' into dockerfile-fix

38560e9

haani-niyaz mentioned this pull request Jul 12, 2022

V0.2.0 review updates #46

Merged

pallasite99 reviewed Jul 13, 2022

View reviewed changes

ghost reviewed Jul 17, 2022

View reviewed changes

Mridul Ganga and others added 3 commits July 18, 2022 19:24

Merge branch 'main' into dockerfile-fix

5e87c80

refactor(Dockerfile): Remove redundant comment

452aa49

refactor(Dockerfile): Fix comment

84afbb1

haani-niyaz changed the base branch from main to v0.2.0-review-updates July 26, 2022 08:24

haani-niyaz and others added 2 commits July 26, 2022 18:26

Merge branch 'v0.2.0-review-updates' into dockerfile-fix

ed33a5b

[pre-commit.ci] auto fixes from pre-commit.com hooks

4d9a96a

for more information, see https://pre-commit.ci

haani-niyaz merged commit a28f2ef into v0.2.0-review-updates Jul 26, 2022

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

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

Uh oh!

Conversation

mridulganga commented Jul 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haani-niyaz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haani-niyaz Jul 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haani-niyaz commented Jul 26, 2022

Uh oh!

Uh oh!

mridulganga commented Jul 7, 2022 •

edited

Loading

haani-niyaz Jul 10, 2022 •

edited

Loading