-
Notifications
You must be signed in to change notification settings - Fork 1k
Conversation
|
docs/FAQ.md
Outdated
|
||
FROM golang:1.8 AS builder | ||
|
||
RUN apt-get update && apt-get install -y unzip --no-install-recommends && \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
New releases will be available as bare binary (not in zip) so there is no need to install unzip. sha256 sum will also be available with binary, so no need to hardcode hash below. Most probably downloaded binary will need execute permission chmod +x
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't wait to be able to link people to this FAQ! 💖
docs/FAQ.md
Outdated
@@ -26,6 +26,7 @@ Summarize the question and quote the reply, linking back to the original comment | |||
* [How does `dep` handle symbolic links?](#how-does-dep-handle-symbolic-links) | |||
* [Does `dep` support relative imports?](#does-dep-support-relative-imports) | |||
* [How do I make `dep` resolve dependencies from my `GOPATH`?](#how-do-i-make-dep-resolve-dependencies-from-my-gopath) | |||
* [How do I use `dep` with Docker?](#how-do-i-make-dep-resolve-dependencies-from-my-gopath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link should point to #how-do-i-use-dep-with-docker
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
docs/FAQ.md
Outdated
|
||
COPY Gopkg.toml Gopkg.lock ./ | ||
|
||
RUN dep ensure -vendor-only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a comment that explains that at this point, vendor/
has been cached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added more comments to explain
docs/FAQ.md
Outdated
|
||
RUN dep ensure -vendor-only | ||
COPY . . | ||
RUN CGO_ENABLED=0 GOOS=linux go build -ldflags "-s -w" -a -installsuffix cgo -o *** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add a comment with a link to more info on what black magic is being used, specifically ldflags
, -a
, and installstuffix cgo
. This has a mix of recommendations floating around on the net and it's not clear what is strictly needed to run in docker, for example why did we omit the netgo workaround as well?
I'm not sure what the best reference is, but https://blog.docker.com/2016/09/docker-golang/, at least explains some of the concerns of running a go binary in a container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I generally use these flags, but if a project needs cgo support this will not work.
CGO_ENABLE=0
disables cgo so this will automagically use netgo 😉.
Build options related to the line are as follows:
-a
force rebuilding of packages that are already up-to-date.
-installsuffix suffix
a suffix to use in the name of the package installation directory,
in order to keep output separate from default builds.
If using the -race flag, the install suffix is automatically set to race
or, if set explicitly, has _race appended to it. Likewise for the -msan
flag. Using a -buildmode option that requires non-default compile flags
has a similar effect.
-ldflags 'flag list'
arguments to pass on each go tool link invocation.
Since we are building go only version(CGO_ENABLED=0
), we also need to build packages that we depend with cgo disabled, so -a
(rebuild even if up to date) and to not mix with previously build version of the same package installsuffix cgo
(use cgo suffix in installation directory).
Linker options are used to make binary small by striping unnecessary symbols.
-s
Omit the symbol table and debug information.
-w
Omit the DWARF symbol table.
On the other hand, this FAQ might not be place to explain these options. It might be better to use bare go build
and using alpine based image for both stages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going to include all those flags, quick comments or links to explain the magic are required IMO. If there is a simpler example that only demonstrates the dep aspect, then that would be preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decided to only use the -o flag, and I added a short comment why.
Add more comments to the sample docker file. Remove most flags in the docker build command to prevent confusion.
Made some changes, please review again @carolynvs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this under "Best Practices" since the behavior section intended to explain why dep does this crazy things it does. Everything else is 💯.
Once you have that fixed, I will merge after v0.3.1 is released.
Move the section on how to use dep with docker from under the Behaviour section to under Best practices section
Moved @carolynvs |
|
||
## How do I use `dep` with Docker? | ||
|
||
`dep ensure -vendor-only` creates the vendor folder from a valid `Gopkg.toml` and `Gopkg.lock` without checking for Go code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, dep ensure -vendor-only
care about Gopkg.lock
only. But it still requires Gopkg.toml
to exists. 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that threw me off too for a sec but the FAQ is less about "how things are exactly implemented" and more high level, "you need both a toml and lock in order for this to work" type stuff. 😀
I vote for leaving it as-is for now since both files are required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Should be merged as soon as we have a new release. 😁
@carolynvs i think your review condition has been satisfied - can you approve? i'm hoping to merge the case sensitivity stuff tonight. |
docs/FAQ.md
Outdated
|
||
Sample dockerfile: | ||
|
||
FROM golang:1.8 AS builder |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to update base image to golang:1.9-alpine
. (alpine
since other stage use alpine and c library may cause error.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer that the end image use scratch
instead.
If you need to use Go in a container you should know how to use the correct image that satisfies your dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How to run static go binaries in Docker isn't the responsibility of the dep FAQ. The question that we want to answer is only the part about caching the results of dep ensure
. If this is going to trigger a "static binary in a docker container" bikeshed, or if we are worried that we have to represent a perfect best practice example in our FAQ, then I vote we rip this out and only show an example up to running dep ensure
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. Removing the parts after dep ensure
is a great idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree too. Going to go ahead and do that.
@sdboyer This is good to go as soon as you cut a new release. |
docs/FAQ.md
Outdated
|
||
FROM golang:1.8 AS builder | ||
|
||
RUN curl -fsSL -o /usr/local/bin/dep https://github.com/golang/dep/releases/download/vX.X.X/dep-linux-amd64 && chmod +x /usr/local/bin/dep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, just came over the docker file issue. Might wanna add
RUN curl -fsSL -o /tmp/dep.zip \
https://github.com/golang/dep/releases/download/v0.3.0/dep-linux-amd64.zip \
&& unzip /tmp/dep.zip -d /usr/local/bin \
&& chmod +x /usr/local/bin/dep
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is waiting on our new release scheme, which won't use zipped binaries on our GitHub releases.
Removes everything after dep ensure -vendor-only This is to avoid oversimplication or ignoring best practices
v0.3.1 has been cut so this is ready go |
What does this do / why do we need it?
Documents how to use
dep
with Docker.What should your reviewer look out for in this PR?
Changes to the FAQ. Whether the documentation properly explains how it can be used.
Do you need help or clarification on anything?
I followed mostly what was said by @ebati on this pull request.
I'd like to know if there is any other use case I should document.
Which issue(s) does this PR fix?
fixes #974.