initiate go module #1150

wingyplus · 2019-09-11T04:35:01Z

Updates #1070

CLAassistant · 2019-09-11T04:35:38Z

All committers have signed the CLA.

codecov-io · 2019-09-11T04:48:31Z

Codecov Report

Merging #1150 into master will decrease coverage by 9.54%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1150      +/-   ##
==========================================
- Coverage   73.61%   64.06%   -9.55%     
==========================================
  Files         144      133      -11     
  Lines       10527    10091     -436     
==========================================
- Hits         7749     6465    -1284     
- Misses       2321     3207     +886     
+ Partials      457      419      -38

Impacted Files	Coverage Δ
lib/options_tls_go1_13.go	`0% <0%> (-100%)`	⬇️
stats/statsd/common/config.go	`0% <0%> (-100%)`	⬇️
stats/statsd/common/api.go	`0% <0%> (-100%)`	⬇️
lib/fsext/cacheonread.go	`0% <0%> (-100%)`	⬇️
lib/netext/httpext/transport.go	`0% <0%> (-96.16%)`	⬇️
lib/netext/httpext/response.go	`0% <0%> (-92.16%)`	⬇️
lib/netext/httpext/request.go	`9.46% <0%> (-87.58%)`	⬇️
js/modules/k6/http/file.go	`0% <0%> (-83.34%)`	⬇️
lib/netext/httpext/digest_transport.go	`0% <0%> (-80%)`	⬇️
lib/netext/httpext/httpdebug_transport.go	`0% <0%> (-75%)`	⬇️
... and 41 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2077dd...e5914fc. Read the comment docs.

cuonglm

We are switching to go module, then I think we can drop vendor directory, and setup proper CI/CD flow to adopt go module.

wingyplus · 2019-09-11T06:05:33Z

Should we need to keep it for Go1.12 ci pipeline. Or use dep ensure to fetch dependencies before running test?

cuonglm · 2019-09-11T06:13:04Z

Should we need to keep it for Go1.12 ci pipeline. Or use dep ensure to fetch dependencies before running test?

I think we can completely remove dep and vendor.

For go1.12 pipeline, we can run:

go mod edit -go=1.12

before running the test.

na-- · 2019-09-11T06:17:03Z

We are switching to go module, then I think we can drop vendor directory, and setup proper CI/CD flow to adopt go module.

Why do you want to drop /vendor? I'd be very reluctant to do so, even with the new official Golang module mirror. I like the safety, predictability, and visibility of having all of the k6 dependencies in our own repo. The visibility is especially useful in helping us avoid very fat or needless dependencies, so that we don't unnecessarily explode the footprint of code we depend on...

Should we need to keep it for Go1.12 ci pipeline. Or use dep ensure to fetch dependencies before running test?

Go modules should work just fine in Go 1.12, we shouldn't need dep there 😕

Also, please don't actually update the dependencies when switching to go modules - it makes this PR very difficult to review. The dependency update should be in a separate pull request, or at the very least, in a separate commit...

cuonglm · 2019-09-11T06:19:52Z

Go modules should work just fine in Go 1.12, we shouldn't need dep there

The go.mod only allows 1 go version.

If we do want to keep vendor directory, then go1.12 pipeline can be run with -mod=vendor 🤔

na-- · 2019-09-11T07:22:03Z

If we have vendor (and I still see no reason to remove it), we should probably use it for all builds - 1.12, 1.13 and so on. For one, given that CI systems will build the project in fresh containers, using the local vendored files will be faster than retrieving them over the network every time...

I had to read through a bunch of Go issues, and to be honest, I'm far from satisfied with how tacked on and poorly supported vendoring is in the go modules world 😞 I loosely followed the Go modules saga, but I never realized that backwards incompatibility, I just assumed things would work as they used to, backwards compatibility guarantees and all... 😞

In any case, it seems like most issues will be fixed in Go 1.14, but until then, we'd have to rely on things like -mod=vendor if we want both go modules and vendoring... Or, more likely, on GOFLAGS=-mod=vendor. Some links if anyone else wants to follow the related discussions:

cmd/go: should default to use mod=vendor flag if the vendor folder exists golang/go#27227: long read, but worth it
cmd/go: allow verifying vendored code golang/go#27348: would have been nice to have as a CI task, like we currently do with dep, but seems like we'd have to wait
cmd/go: the vendor/ folder should be kept up to date if present golang/go#29058: yeah, you don't say... 😄
proposal: cmd/go: automatic and partial vendoring in module mode golang/go#30240: previous proposal for fixing the current deficiencies, with this comment in particular making a ton of sense
cmd/go: automatically check and use vendored packages golang/go#33848: the current proposal, probably what we should follow closely

cuonglm · 2019-09-11T07:57:06Z

@na-- I think we should only use go module for vendor management only, so the process will be:

go mod tidy
go mod vendor

Then in CI/CD, we will explicitly set GO111MODULE=off.

na-- · 2019-09-11T08:07:12Z

👍 seems reasonable, it may be an easier workaround until vendoring is properly supported in Go 1.14, but as I've seen today, I'm far from an expert in Go modules...

wingyplus · 2019-09-11T10:18:43Z

@cuonglm @na-- I think we can use go module mode on go1.12 by using enabling GO111MODULE. This can be the same as setting on go1.13.

cuonglm · 2019-09-11T10:21:07Z

@cuonglm @na-- I think we can use go module mode on go1.12 by using enabling GO111MODULE. This can be the same as setting on go1.13.

Yes, you can.

But the issue is that go.mo specify go1.13 version, so without editing go.mod, building using go1.12 will fail. That's why I said it's better to use go module for vendoring only, then in CI/CD, just turn off GO111MODULE and build using vendor.

wingyplus · 2019-09-11T10:22:00Z

I found test failing on go1.13 in the current build of this pr:

--- FAIL: TestSentReceivedMetrics (36.73s)
    --- FAIL: TestSentReceivedMetrics/group (0.00s)
        --- FAIL: TestSentReceivedMetrics/group/SentReceivedMetrics_script[3]_case[4](25,2) (3.11s)
            engine_test.go:624: noReuseReceived=28325.000000 is greater than reuseReceived=28329.000000

Not sure is it flaky test?

wingyplus · 2019-09-11T10:25:01Z

@cuonglm I think we edit go.mod to go 1.12 at minimum version to support both version in CI. And we'll bump to go 1.13 when go 1.14 release. What do you think?

cuonglm · 2019-09-11T10:27:45Z

@cuonglm I think we edit go.mod to go 1.12 at minimum version to support both version in CI. And we'll bump to go 1.13 when go 1.14 release. What do you think?

Ah, sound good 👍

And also, the test false is flaky one, see ##1149

wingyplus · 2019-09-11T10:34:18Z

@cuonglm Thanks for your pointing out an issue. I'll wait until it fix and rebase to test again if #1149 got merged to master. :)

wingyplus · 2019-09-11T10:35:49Z

test-prev-golang task work as expect 🎉

thinkerou · 2020-05-24T01:28:07Z

hi any updates? merged before #1007 ? thanks!

na-- · 2020-05-27T11:21:26Z

@thinkerou, moving to use go modules will definitely have to wait until after #1007 is merged... Maybe we'll do it in conjunction with #959.

Also, we want to keep vendoring our dependencies, so we had to wait for Go 1.14 (see linked golang issues in #1150 (comment)), and this is something that this PR doesn't address, so when the time comes, we'll probably close this and make a new PR.

thinkerou · 2020-05-27T11:52:56Z

@na-- got it, thank your reply!

imiric · 2020-08-14T11:09:19Z

@wingyplus Thanks for your work on this, but we did it from scratch in #1584, which is now merged, so closing this.

initiate go module

9c0556b

cuonglm reviewed Sep 11, 2019

View reviewed changes

na-- mentioned this pull request Sep 11, 2019

Adopt Go modules #1070

Closed

wingyplus force-pushed the gomod branch from 7c93725 to edd84b7 Compare September 11, 2019 09:18

wingyplus added 10 commits September 11, 2019 17:59

go mod edit -go=1.12

4b32f6b

Remove dep task in circleci and vendor directory

58d7ac2

Remove Gopkg.{toml,lock}

b07259e

Remove deps job in circleci workflow

a906ccf

exporting GO111MODULE manually

e6e870a

Refactor coverage on job go1.13

1f8d41a

Try remove coverage package on go1.13

e4a0ba6

Bring coverage back

aad790f

Coverage each package test and combine later

458af8f

Fix indent and bring -race back

ff0651d

wingyplus added 2 commits September 11, 2019 18:00

Replace coverpkg to use $pkg instead of go list

5728f37

Enable GO111MODULE on appveyor

e5914fc

wingyplus force-pushed the gomod branch from b3cba0c to e5914fc Compare September 11, 2019 11:01

na-- added this to the v0.26.0 milestone Sep 13, 2019

na-- modified the milestones: v0.26.0, v0.27.0 Oct 10, 2019

na-- modified the milestones: v0.27.0, v0.28.0 May 21, 2020

imiric closed this Aug 14, 2020

wingyplus deleted the gomod branch August 14, 2020 11:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initiate go module #1150

initiate go module #1150

wingyplus commented Sep 11, 2019 •

edited

Loading

CLAassistant commented Sep 11, 2019 •

edited

Loading

codecov-io commented Sep 11, 2019 •

edited

Loading

cuonglm left a comment

wingyplus commented Sep 11, 2019 •

edited

Loading

cuonglm commented Sep 11, 2019

na-- commented Sep 11, 2019

cuonglm commented Sep 11, 2019

na-- commented Sep 11, 2019 •

edited

Loading

cuonglm commented Sep 11, 2019

na-- commented Sep 11, 2019

wingyplus commented Sep 11, 2019

cuonglm commented Sep 11, 2019

wingyplus commented Sep 11, 2019

wingyplus commented Sep 11, 2019

cuonglm commented Sep 11, 2019

wingyplus commented Sep 11, 2019

wingyplus commented Sep 11, 2019

thinkerou commented May 24, 2020

na-- commented May 27, 2020

thinkerou commented May 27, 2020

imiric commented Aug 14, 2020

initiate go module #1150

initiate go module #1150

Conversation

wingyplus commented Sep 11, 2019 • edited Loading

CLAassistant commented Sep 11, 2019 • edited Loading

codecov-io commented Sep 11, 2019 • edited Loading

Codecov Report

cuonglm left a comment

Choose a reason for hiding this comment

wingyplus commented Sep 11, 2019 • edited Loading

cuonglm commented Sep 11, 2019

na-- commented Sep 11, 2019

cuonglm commented Sep 11, 2019

na-- commented Sep 11, 2019 • edited Loading

cuonglm commented Sep 11, 2019

na-- commented Sep 11, 2019

wingyplus commented Sep 11, 2019

cuonglm commented Sep 11, 2019

wingyplus commented Sep 11, 2019

wingyplus commented Sep 11, 2019

cuonglm commented Sep 11, 2019

wingyplus commented Sep 11, 2019

wingyplus commented Sep 11, 2019

thinkerou commented May 24, 2020

na-- commented May 27, 2020

thinkerou commented May 27, 2020

imiric commented Aug 14, 2020

wingyplus commented Sep 11, 2019 •

edited

Loading

CLAassistant commented Sep 11, 2019 •

edited

Loading

codecov-io commented Sep 11, 2019 •

edited

Loading

wingyplus commented Sep 11, 2019 •

edited

Loading

na-- commented Sep 11, 2019 •

edited

Loading