-
-
Notifications
You must be signed in to change notification settings - Fork 658
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 module definition #630
Conversation
Also updates CI configuration to be compatible with modules. Fixes onsi#629
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.
@nmiyake thank you for this contribution. It's definitely a step in the right direction.
I have some questions/concerns about a few aspects, and I'd be interested in hearing your response to them. Because this is such an overdue change, I'm inclined to merge it, but wanted to have a quick discussion first.
@@ -4,14 +4,23 @@ go: | |||
- 1.13.x | |||
- tip | |||
|
|||
cache: |
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.
What's the rationale for using a cache? I'm not an expert on Travis caching, but I imagine there's a trade-off between independence of builds and speed. Is there a sense of how much time is saved, and how often the cache is invalidated?
- go get -v -t ./... | ||
- go get golang.org/x/tools/cmd/cover | ||
- go get github.com/onsi/gomega | ||
- GO111MODULE="off" go get -v -t ./... |
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.
These changes ensure that everything still works in the old way. One thing that I'm a little uncomfortable with is that the version of (say) gomega
is defined in the go.mod
file, but these commands will get the tip of the master
branch - which might be different.
I'd be interested in thinking about whether it makes sense to use the Go Modules mechanism of downloading 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.
Follow-up here: enabling modules more broadly causes issues with the ginkgo execution itself, with tests failing due to not having module definitions etc. I haven't had a chance to dig in deeply yet, but it looks like making that work will potentially require further changes beyond just the Travis setup.
- go install github.com/onsi/ginkgo/ginkgo | ||
- export PATH=$PATH:$HOME/gopath/bin | ||
|
||
script: $HOME/gopath/bin/ginkgo -r --randomizeAllSpecs --randomizeSuites --race --trace && go vet | ||
script: | ||
- GO111MODULE="on" go mod tidy |
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 really like these checks. It's a pattern that I'll use elsewhere.
go.mod
Outdated
@@ -0,0 +1,9 @@ | |||
module github.com/onsi/ginkgo | |||
|
|||
go 1.13 |
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 know that this line is inserted automatically. I would rather it was set at the lowest version of Go supported (currently v1.12)
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.
Yes, makes sense, will update.
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.
Thanks for the feedback! Will update the PR as follows:
- Declare Go 1.12 in
go.mod
- Remove build caching (just cache module directory)
- Experiment with making the verify steps more module-friendly
go.mod
Outdated
@@ -0,0 +1,9 @@ | |||
module github.com/onsi/ginkgo | |||
|
|||
go 1.13 |
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.
Yes, makes sense, will update.
@nmiyake, sorry it's taken so long to get round to looking at this again. Thank you for your contribution! |
@blgm Would you release a new version of ginkgo that uses go modules? |
Hi @mpolanski, v1.12.0 has just been released! |
Also updates CI configuration to be compatible with modules.
Fixes #629