Conversation
885b7b1
to
b0c284b
Compare
Pull Request Test Coverage Report for Build 3353
💛 - Coveralls |
As of go 1.11, sql _does_ return context errors in this case, so this test no longer provides value. golang/go@16f32a0
In previous go versions, `string(1)` happily produced the rune, "\x01". Now, it produces this compiler warning: [stringintconv] [W] conversion from untyped int to string yields a string of one rune, not a string of digits (did you mean fmt.Sprint(x)?) I updated this test data to use strings closer to what one might expect, ("1" rather than "\x01").
Some issue creators noted that we only thunder in go 1.10. Since we use go 1.15 in production, this seems unwise. This commit sets the go version to 1.15. It also makes a change to use go modules rather than the vendor folder. I don't think we can make one change without the other, but I know very little about go modules and I'd love to be mistaken. XXX: I know our setup is a bit weird, and I'm a bit concerned that we'll have to make changes to internal repos next time we revendor thunder. Maybe we can add it as a module? That would be neat. I'm inclined to merge this change, then figure that out. ********************************************************************************** commands: ```bash go mod init // This signaled an error wrt. an "appengine" thing in vendor.json // I made the changes below to vendor.json, then tried again. go mod vendor rm -rf vendor ``` vendor.json: ``` "ignore": "appengine appenginevm test", // I removed appengine and appenginevm. "package": [ { // I removed this whole package entry. "path": "appengine/cloudsql", "revision": "" }, ```
b0c284b
to
29cf1bc
Compare
Pinging you as resident go-modules knower, particularly w.r.t the note under XXX in the PR description. |
Pinging you as thunder maintainer apparent. I think testing in 1.15 should be uncontroversial, and the test changes should be uncontroversial, but I don't know what to make of the go modules change. See the note under XXX in the PR description. |
We shouldn't have to do anything special with Thunder for internal use, we should be able to make use of the package regardless of whether it is module-ised (modularised?) or not. The core code hasn't changed, and currently performs fine with all our vendored dependencies, so I don't think upgrading would be a headache. Removing the strange appengine vendoring LGTM. It seems this dependency was only present for the mysql driver:
This was dependency removed in go-sql-driver/mysql#1007, and was apparently only needed for Go <=1.9, so this should be totally fine to remove. |
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 approve this message
Some issue creators noted that we only test thunder in go 1.10. Since we use go 1.15
in production, this seems unwise. This commit sets the go version to 1.15. It
also makes a change to use go modules rather than the vendor folder. I don't
think we can make one change without the other, but I know very little about go
modules and I'd love to be mistaken.
XXX: I know our setup is a bit weird, and I'm a bit concerned that we'll have
to make changes to internal repos next time we revendor thunder. Maybe we can
add it as a module? That would be neat. I'm inclined to merge this change, then
figure that out.
commands:
vendor.json:
There are also two commits that fix small build/test errors present when
running with go1.15.