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
Add Dockerfile to test fargo #50
Conversation
WORKDIR /go/src/github.com/hudl/fargo/ | ||
RUN godep restore | ||
|
||
#UNcomment if you want to run tests on master branch. |
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.
- s/#UN/# Un/
@@ -0,0 +1,12 @@ | |||
FROM golang:1.6 |
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.
- That's a conservative version at this point. Would you consider using version 1.7?
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.
Fargo should work with go version 1.7 but since we are testing all our go software using 1.6 it makes more sense to stick with that version. I agree that we could use newer version for just testing fargo, but I don't see any benefits from running it on version 1.7.
RUN godep restore | ||
|
||
#UNcomment if you want to run tests on master branch. | ||
#CMD go test -v ./... |
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 the point of this file is to run tests, why comment this out here?
- Ideally we'd have a way (perhaps via
testing.Short
) to avoid the network-dependent tests.
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.
Commented out as this checks out master branch of fargo. I just updated the Readme to say checkout the test branch then run tests.
Regarding the IP addresses, yes unfortunately those IPs are hardcoded in tests, so I will let anyone familiar with fargo and tests fix that.
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.
Right now most tests require eureka servers running, this is not ideal, I know. But we want to make it easier to actually run those tests. For now we are trying MVP just to run tests once.
We may remove that comment entirely probably now when you mention that. All instructions are in README.md now.
ENV GOROOT=/usr/local/go | ||
|
||
RUN go get github.com/tools/godep | ||
RUN go get github.com/hudl/fargo |
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.
- Why not use the source tree available here? The
onbuild
image makes it even easier.
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.
Can you please give as a hint how to make it work? I'm new to docker :(
I've tried:
docker run --rm -v "$PWD":/usr/src/github.com/hudl/fargo -w /usr/src/github.com/hudl/fargo golang:1.6 go test ./...
But it gives me :
Error response from daemon: Invalid bind mount spec "/C/Users/damian.turczynski/Documents/hudl/go/src/github.com/hudl/fargo;C:\\Program Files\\Git\\usr\\src\\github.com\\hudl\\fargo": Invalid volume destination path: '\Program Files\Git\usr\src\github.com\hudl\fargo' mount path must be absolute..
I love the idea though.
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.
Since I'm using GitBash for windows MSYS_NO_PATHCONV=1
helped. I'll try to make it work as you wanted @seh
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.
Ok, this will not work on Docker for Windows yet ... Probably worth changing it later moby/moby#22981
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 you look at the onbuild
Dockerfile, you can see that it copies the current directory tree into the directory /go/src/app inside the container, then takes care of go get and go build for any dependencies of the copied source tree.
It seems strange here to fetch the fargo source from GitHub when a developer interested in running tests is probably trying to check whether his local changes broke anything—say, before submitting a PR.
Perhaps I misunderstand the intended usage of this image.
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.
@seh: I haven't been following all of this, but I'd tend to agree. Another scenario: if we want to set this up to automatically run tests through Docker somewhere, it seems logical for it to use the latest source for each branch we're testing.
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 think we can issue a PR for that and merge it as soon as Docker will add support for Windows, before that I feel we should stick to working on all platforms solution
Add docker file and docker build instructions