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

Fill the docker section #42

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

Fill the docker section #42

wants to merge 3 commits into from

Conversation

nhurel
Copy link

@nhurel nhurel commented Aug 4, 2017

Proposal for your issue #5

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@nhurel
Copy link
Author

nhurel commented Aug 4, 2017

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

Copy link
Owner

@campoy campoy left a comment

Choose a reason for hiding this comment

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

Hey, thanks so much for this! I sent a couple of comments, I'll send more once you've done these edits.

- Local development

### Continuous integration
When using a CI server like Jenkins, it can become cumbersome to have to install all the go versions required by your different projects. And even more if you use your CI server to build projects in other languages, each with its own version (java8, java7, node, ruby, python, ...). For this use case, it can be easier to build the projects in docker containers. An example script could be :
Copy link
Owner

Choose a reason for hiding this comment

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

please break this into lines for easier review


### Continuous integration
When using a CI server like Jenkins, it can become cumbersome to have to install all the go versions required by your different projects. And even more if you use your CI server to build projects in other languages, each with its own version (java8, java7, node, ruby, python, ...). For this use case, it can be easier to build the projects in docker containers. An example script could be :
```
Copy link
Owner

Choose a reason for hiding this comment

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

'''bash
$ docker ...

### Continuous integration
When using a CI server like Jenkins, it can become cumbersome to have to install all the go versions required by your different projects. And even more if you use your CI server to build projects in other languages, each with its own version (java8, java7, node, ruby, python, ...). For this use case, it can be easier to build the projects in docker containers. An example script could be :
```
docker run --rm -v $PWD:/go/src/github.com/repo/project -w /go/src/github.com/repo/project golang:1.8.1 go build -o dist/binary
Copy link
Owner

Choose a reason for hiding this comment

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

Why the -o dist/binary?

Copy link
Author

Choose a reason for hiding this comment

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

because copy/pasting from another script 😇 I'll remove the option

This will run the `go build` command with go version 1.8.1, sharing your current directory with the container. Obviously, you can run `go fmt/vet/test/...` commands the same way

### Cross building binary
Usually, cross building a binary requires to install go from sources. Using the golang image, you can install the binary distribution on your computer for day-to-day use and rely on the docker image to build binaries for different targets.
Copy link
Owner

Choose a reason for hiding this comment

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

This is wrong, you can cross compile by installing Go with an installer to install from source

Copy link
Author

Choose a reason for hiding this comment

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

OK, I don't know how to do that TBH. Maybe my use case of using the golang image to generate binary is not a good example and you want to drop it ?

```
This will run the `go build` command with go version 1.8.1, sharing your current directory with the container. Obviously, you can run `go fmt/vet/test/...` commands the same way

### Cross building binary
Copy link
Owner

Choose a reason for hiding this comment

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

I expect this section to talk about how you can do

$ GOOS=linux go build
$ docker build .

Where Dockerfile adds the linux binary

Copy link
Author

Choose a reason for hiding this comment

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

hmmm... sorry don't understand what you mean here. The use case I have in mind in this section is only to use docker run to generate a binary, nothing more. Building an image by adding the binary in the Dockerfile is shown as an example in the sections below.

add bash syntax highlighting to command scripts
@nhurel
Copy link
Author

nhurel commented Aug 11, 2017

I've integrated part or your remarks. I don't fully understand what you expect from the Cross building binary section, I let you read my comments.

@campoy
Copy link
Owner

campoy commented Aug 23, 2017

Hey, I just came back from holidays and I'll be reviewing this in a couple days. Thanks so much!

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.

None yet

3 participants