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

Better onboarding #161

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Better onboarding #161

wants to merge 3 commits into from

Conversation

tyron
Copy link
Contributor

@tyron tyron commented Jan 19, 2022

  • explains how to use PostgreSQL image and launch Doc images
  • optionally exposing Gitter host/port so it can be used locally with Docker
  • improved Dockerfile by allowing cache of Go dependencies (by moving copy of go.sum and go.mod before go mod download)

Comment on lines +59 to +60
gitterHostDefault = "127.0.0.1"
gitterPortDefault = "1234"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code updated below to use environment variables (so that this code can run in a docker aside the gitter runner), and these defaults were kept for backwards compatibility.

Copy link
Member

@hasheddan hasheddan left a comment

Choose a reason for hiding this comment

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

Thanks so much for the contribution @tyron and my apologies for the delayed review! Mostly looks good, I think we can drop the Dockerfile changes though 👍🏻

Comment on lines -8 to +10
# Copy internal libraries.
COPY . .
# Copy and cache dependencies
COPY ../go.mod .
COPY ../go.sum .
Copy link
Member

Choose a reason for hiding this comment

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

This Dockerfile typically gets invoked from root via the Makefile so I think we can keep this as is (the proposed change will not work with make build-doc or make build-gitter.

Copy link
Contributor Author

@tyron tyron Feb 19, 2022

Choose a reason for hiding this comment

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

make build-doc and make build-gitter both work for me. Did you have any problems?
Up to you, I just feel these changes make it easier to iterate during development.

Analyzing execution of make build-doc...
Based on the main branch, the 1st execution took me 78s. Another execution on top of that without any changes was quick, 1.8s. Then I just added a comment to deploy/doc.yaml (echo "\n# test" >> deploy/doc.yaml), increased timing to 56s.

Now with my proposed changes, the 2 first executions were very similar. But the 3rd execution, which has an updated file, took just 13s (vs 56s).

Again, I'm not working often with the repo to say if that's important. But when I had to build the image several times, that saved me quite some time

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just for reference, running these before go mod download is suggested in golang's Dockerhub under "Start a Go instance in your app"

Copy link
Member

Choose a reason for hiding this comment

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

@tyron yep I am on board with copying just the go.mod / go.sum files in for go mod download to ensure that we can cache and re-use the intermediate image 👍🏻 The Makefile is in the root of the directory though, as is the build context, so your relative paths to ../go.mod and ../go.sum should just be go.mod and go.sum.

Copy link
Member

Choose a reason for hiding this comment

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

FWIW we can also mount the go build cache as well if you want to speed it up even more :)

Copy link
Contributor Author

@tyron tyron Feb 20, 2022

Choose a reason for hiding this comment

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

Hey, so I tested a bit more because I was curious on how my setup was working.

I believe the notation in the Dockerfile is not based on your current pwd, yet relative to Dockerfile itself. From what I can test, even if your build context is the root folder and the go.mod is in the root folder, it should still be written as ../go.mod if Dockerfile is in a subfolder.

So since go.{mod,sum} are always on the parent folder of deploy, writing as ../go.{mod,sum} always returns the proper location. Does it makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, now I don't understand anything. Both COPY ../go.mod . and COPY go.mod . seem to work.

@@ -3,15 +3,19 @@
# https://hub.docker.com/_/golang
FROM golang:1.13 as builder

WORKDIR app/
WORKDIR /go/app/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is following best-practices of using an absolute path instead of relative.

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.

2 participants