Skip to content
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.

Sign up for GitHub

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

Jump to bottom

*: migrate dependency management to Go1.11 module #8054

Merged
merged 12 commits into from
Nov 1, 2018
Merged

*: migrate dependency management to Go1.11 module #8054

merged 12 commits into from
Nov 1, 2018

Conversation

tiancaiamao
Copy link
Contributor

What problem does this PR solve?

Fix #7922

What is changed and how it works?

  • go mod init
  • remove vendor directory
  • rename import path "github.com/pkg/errors" to "github.com/pingcap/errors"
  • tiny update util/logutil to fix CI

Check List

Tests

  • No code

Side effects

  • Don't forget to update other repos

@disksing @lysu

disksing
disksing reviewed Oct 25, 2018
View reviewed changes
Makefile Outdated

build:
$(GOBUILD)

# The retool tools.json is setup from hack/retool-install.sh
check-setup:
@which retool >/dev/null 2>&1 || go get github.com/twitchtv/retool
@retool sync
@GO111MODULES=off retool sync
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GO111MODULE

@tiancaiamao
Copy link
Contributor Author

/run-all-tests

@winoros winoros added the type/enhancement The issue or PR belongs to an enhancement. label Oct 25, 2018
@tiancaiamao
Copy link
Contributor Author

/run-all-tests tidb-test=pr/639

@tiancaiamao
Copy link
Contributor Author

/run-all-tests tidb-test=pr/639

@tiancaiamao
Copy link
Contributor Author

I nearly make all test pass, but after merge master and resolve conflicts, I find tidb-tools have introduce module and it use a pd version conflict with here.

@tiancaiamao
Copy link
Contributor Author

/run-all-tests tidb-test=pr/639

1 similar comment
@tiancaiamao
Copy link
Contributor Author

/run-all-tests tidb-test=pr/639

@tiancaiamao
Copy link
Contributor Author

/run-common-test tidb-test=pr/639

@tiancaiamao
Copy link
Contributor Author

/run-all-tests tidb-test=pr/639

@tiancaiamao
Copy link
Contributor Author

/run-common-test tidb-test=pr/639

lysu
lysu reviewed Nov 1, 2018
View reviewed changes
Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

jackysp
jackysp approved these changes Nov 1, 2018
View reviewed changes
Copy link
Member

@jackysp jackysp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gregwebs
Copy link
Contributor

gregwebs commented Nov 1, 2018

One thing that can help avoid disrupting workflows is to do the switch but to use go mod's vendoring support (we did this in pd).
Even if we don't want to commit that, it is very helpful for code review.

@tiancaiamao
Copy link
Contributor Author

Good suggestion, we can adopt it in tidb-tools. @gregwebs
vendor is not included here because:

  1. tidb update parser frequently, and that will make this repo expansion quickly
  2. vendor is not recommend by the Go team

@tiancaiamao tiancaiamao merged commit b7f431e into pingcap:master Nov 1, 2018
@tiancaiamao tiancaiamao deleted the go-module branch November 1, 2018 12:54
gregwebs
gregwebs reviewed Nov 1, 2018
View reviewed changes
go.mod
google.golang.org/appengine v1.2.0 // indirect
google.golang.org/genproto v0.0.0-20181029155118-b69ba1387ce2 // indirect
google.golang.org/grpc v1.16.0
gopkg.in/airbrake/gobrake.v2 v2.0.9 // indirect
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for?

@gregwebs
Copy link
Contributor

gregwebs commented Nov 1, 2018

I think go.mod needs a more thorough review still. We may have a repeat of the same issue we had in PD of pulling in some odd dependencies tikv/pd#1269 (review)
Talk with @disksing

@tiancaiamao
Copy link
Contributor Author

Yeah, the hardest step is resolving the dependency and make CI work.
#7922

Now it's time to polish it, feel free to send some PRs. @gregwebs

@gregwebs
Copy link
Contributor

gregwebs commented Nov 1, 2018

I won't be sending PRs any time soon. You should be able to ask @disksing what he did.

disksing
disksing reviewed Nov 2, 2018
View reviewed changes
go.mod
github.com/cockroachdb/cmux v0.0.0-20170110192607-30d10be49292 // indirect
github.com/codahale/hdrhistogram v0.0.0-20161010025455-3a0bb77429bd // indirect
github.com/coreos/bbolt v1.3.1-coreos.6 // indirect
github.com/coreos/etcd v3.3.10+incompatible
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why there is an incompatible? Does it mean that we have dependencies that import different versions of etcd?

@disksing
Copy link
Contributor

disksing commented Nov 2, 2018 •
edited
Loading

@gregwebs In fact, in the case where the vendor is deleted, it is difficult to know the difference between the dependencies used by go.mod and the previous vendor dependencies. This PR may have inadvertently changed a lot of dependent versions.

@gregwebs gregwebs mentioned this pull request Nov 20, 2018
Closed
@tiancaiamao tiancaiamao mentioned this pull request Nov 24, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregwebs gregwebs gregwebs left review comments

@lysu lysu lysu left review comments

@disksing disksing disksing left review comments

@jackysp jackysp jackysp approved these changes

Assignees
No one assigned
Labels
type/enhancement The issue or PR belongs to an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support Go1.11 module
6 participants
@tiancaiamao @gregwebs @disksing @lysu @jackysp @winoros