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

go mod: update the parser version #8632

Merged
merged 7 commits into from
Dec 10, 2018
Merged

go mod: update the parser version #8632

merged 7 commits into from
Dec 10, 2018

Conversation

winkyao
Copy link
Contributor

@winkyao winkyao commented Dec 10, 2018 •
edited by c4pt0r
Loading

What problem does this PR solve?

It solved the mistake imported by #8037, I merge the TiDB pr before the pingcap/parser#14 merged.

Maybe we should find a way to avoid this thing happen again.

This change is Reviewable

@winkyao
Copy link
Contributor Author

winkyao commented Dec 10, 2018

@winoros @zz-jason PTAL

winoros
winoros reviewed Dec 10, 2018
View reviewed changes
Copy link
Member

@winoros winoros left a comment

Choose a reason for hiding this comment

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

lgtm

@zz-jason
Copy link
Member

Should we add a check item in the Makefile?

Return error if replace github.com/pingcap/parser is found in go.mod. Thus the PRs which replace pingcap/parser to another parser can not pass CI, then it can not be merged automatically, then this kind of mistake will no longer happen.

@winkyao
Copy link
Contributor Author

winkyao commented Dec 10, 2018

@zz-jason But we need the replace to verify the merging parser pr is work.

@zz-jason
Copy link
Member

zz-jason commented Dec 10, 2018 •
edited
Loading

@zz-jason But we need the replace to verify the merging parser pr is work.

You can add it as the last step of check:

 57 dev: checklist test check

 ...
 67 check: check-setup fmt lint vet #thirdparser

This check progress will be started once all the unit tests are passed

Or you can add this check in circle.yml, like go mod tidy, see #8402 for more detail.

@zz-jason zz-jason added status/LGT1 Indicates that a PR has LGTM 1. component/server labels Dec 10, 2018
@dbjoa
Copy link
Contributor

dbjoa commented Dec 10, 2018

@winkyao master branch has been broken. Could you recover your local branch github.com/winkyao/parser v0.0.0-20181207053352-37ef416fe2a0 ?

CGO_ENABLED=0 GO111MODULE=on go build   -ldflags '-X "github.com/pingcap/parser/mysql.TiDBReleaseVersion=v2.1.0-rc.3-300-g3feb22b" -X "github.com/pingcap/tidb/util/printer.TiDBBuildTS=2018-12-10 06:37:20" -X "github.com/pingcap/tidb/util/printer.TiDBGitHash=3feb22b291229ac5bcef39bcbc8a5428a92ed6bc" -X "github.com/pingcap/tidb/util/printer.TiDBGitBranch=fix-the-issue-8615" -X "github.com/pingcap/tidb/util/printer.GoVersion=go version go1.11 linux/amd64" ' -o bin/tidb-server tidb-server/main.go
go: finding github.com/winkyao/parser v0.0.0-20181207053352-37ef416fe2a0
go: github.com/winkyao/parser@v0.0.0-20181207053352-37ef416fe2a0: unknown revision 37ef416fe2a0
go: error loading module requirements
Makefile:161: recipe for target 'server' failed
make: *** [server] Error 1

@winkyao
Copy link
Contributor Author

winkyao commented Dec 10, 2018

@dbjoa Fine, I push the local branch to the github. Please try to rerun the unit tests.

@dbjoa
Copy link
Contributor

dbjoa commented Dec 10, 2018

OK, recovered!

@winkyao
Copy link
Contributor Author

winkyao commented Dec 10, 2018

@zz-jason Well, I'll figure it out.

@winoros
Copy link
Member

winoros commented Dec 10, 2018

In pingcap/parser#67, DropViewStmt is not a fake stmt any more. So the plan generated from it is not TableDual with Projection any more.
Modify the test to fit the current situation.

XuHuaiyu
XuHuaiyu reviewed Dec 10, 2018
View reviewed changes
planner/core/logical_plan_test.go
@@ -993,7 +993,7 @@ func (s *testPlanSuite) TestColumnPruning(c *C) {
},
//issue 7833
{
sql: "drop view if exists v",
Copy link
Contributor

Choose a reason for hiding this comment

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

We can just remove it.
We'll add other test cases for drop view later.

zz-jason
zz-jason previously approved these changes Dec 10, 2018
View reviewed changes
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Dec 10, 2018
@zz-jason
Copy link
Member

/run-all-tests

@winkyao
Copy link
Contributor Author

winkyao commented Dec 10, 2018

/run-all-tests

@winkyao winkyao merged commit f597d18 into pingcap:master Dec 10, 2018
@winkyao winkyao deleted the fix_go_mod branch December 10, 2018 08:12
iamzhoug37 pushed a commit to iamzhoug37/tidb that referenced this pull request Dec 13, 2018
@winkyao @iamzhoug37
go mod: update the parser version (pingcap#8632)
7560b8e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XuHuaiyu XuHuaiyu XuHuaiyu left review comments

@winoros winoros winoros left review comments

@zz-jason zz-jason zz-jason approved these changes

Assignees
No one assigned
Labels
component/server status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@winkyao @zz-jason @dbjoa @winoros @XuHuaiyu