-
Notifications
You must be signed in to change notification settings - Fork 266
Conversation
Dockerfile
Outdated
@@ -6,6 +6,9 @@ ENV GOOS=linux | |||
|
|||
RUN mkdir -pv $NOMS_SRC | |||
COPY . ${NOMS_SRC} | |||
RUN cd $NOMS_SRC/cmd/noms; \ | |||
go build; \ | |||
go test |
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.
We should build and test the packages under noms/go
too.
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.
Will do, and I'll update the contributor guide since I cribbed the test commands from there ;)
It'd be cool to test PRs. And it looks like Travis is free for open source projects. Are there any downsides to using Travis that you know of? It's been awhile since I've had to think about it. If not, I'll set it up today/tonight. I agree it still makes sense to land this for the reason you mention. === Side note: in order to land this, I need you to complete the contributors agreement: https://github.com/attic-labs/noms/blob/master/CONTRIBUTING.md#contributing-code |
PS: Thanks for the contributions! |
Signed the agreement while the fork was... forking... so I should be good there, will add the above changes and the |
Might need some tweaking, this is my first
I've not used Travis a lot, been doing more with Drone lately, but Travis still seems pretty handy given the free for OSS and hosted nature of it all. |
Alright @aboodman, I think this is ready for merge. I also pulled samples out of the build image as their test dependencies weren't resolving and it's not really related to noms working. I haven't tested Travis file. Not 100% sure how to do that without just installing it and pushing |
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.
Sweet, thanks!
codecov.yml | ||
CONTRIBUTING.md | ||
LICENSE | ||
README.md | ||
samples |
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.
Nit, but add the trailing newline.
@@ -3,9 +3,11 @@ FROM golang:latest AS build | |||
ENV NOMS_SRC=$GOPATH/src/github.com/attic-labs/noms | |||
ENV CGO_ENABLED=0 | |||
ENV GOOS=linux | |||
ENV NOMS_VERSION_NEXT=1 |
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.
Remove the dupe line further down.
Nits PR here: https://github.com/attic-labs/noms/pull/3811/files |
Regarding #3792
Wanted to start the discussion on picking the low hanging fruit here.
If we'd like to do that I'm happy to write the
.travis.yml
if someone can enable travis for the repoNote: tests have two actual failures at this point so this will fail to build at dockerhub. Accepting this pull as is though will show that failures do get correctly reported in the badge so should perhaps proceed before fixing the two issues.
Even if we move to travis and don't use docker build as the script, which is also fine, it might be worth considering accepting this change so that we don't publish broken code to the
latest
tag on dockerhub.