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

Switch checks to use retool. Fix goword and errcheck #7240

Merged
merged 6 commits into from
Aug 2, 2018
Merged

Switch checks to use retool. Fix goword and errcheck #7240

merged 6 commits into from
Aug 2, 2018

Conversation

gregwebs
Copy link
Contributor

@gregwebs gregwebs commented Aug 1, 2018

What have you changed? (mandatory)

Switch checks to use retool. retool vendors check tools local to the project.
Note that you can also add other tools such as code generators to the retool setup.

Fix goword and errcheck: these were not being run correctly.These are now under the check-fail task.

I would recommend creating an issue to fix the check-fail task. This can be time boxed to some amount of time per week, but will probably take more than one day total. errcheck in particular is of great importance: unhandled errors are almost guaranteed to result in bugs. I can show you how I handle collecting errors in defer statements.

What is the type of the changes? (mandatory)

Improvement/New Feature for static analysis

How has this PR been tested? (mandatory)

make check

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

no

Does this PR affect tidb-ansible update? (mandatory)

no

Does this PR need to be added to the release notes? (mandatory)

Switch static checks to use retool.

gregwebs added 2 commits August 1, 2018 09:31
@gregwebs
use retool for linters
64a837b 
Also fix goword and errcheck which were not being run correctly.
These are now under the check-fail task.
@gregwebs
fix goword wants proper package comments
02baf84
@gregwebs gregwebs added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 1, 2018
@gregwebs gregwebs requested a review from shenli August 1, 2018 16:42
@zz-jason
Copy link
Member

zz-jason commented Aug 2, 2018

CI failed by:

retool: downloading honnef.co/go/tools/cmd/megacheck
retool: fatal err: execution error on "go get -d honnef.co/go/tools/cmd/megacheck": package honnef.co/go/tools/cmd/megacheck: unrecognized import path "honnef.co/go/tools/cmd/megacheck" (parse https://honnef.co/go/tools/cmd/megacheck?go-get=1: no go-import meta tags ())
: failed to 'go get' tool: exit status 1
Makefile:103: recipe for target 'check-setup' failed

@gregwebs
Copy link
Contributor Author

gregwebs commented Aug 2, 2018

That's odd: this is the same as PD which is building just fine. Let me know if you understand this error. Otherwise I will just remove it for now: there are plenty of other checks to deal with right now.
https://github.com/pingcap/pd/blob/master/tools.json
https://github.com/pingcap/pd/blob/master/Makefile#L54

@lysu
Copy link
Contributor

lysu commented Aug 2, 2018

It seems honnef.co unreachable at that moment, so go get couldn't get meta to redirect to git?? I have rerun it and seems to be ok now.

zz-jason
zz-jason reviewed Aug 2, 2018
View reviewed changes
Makefile Outdated
# check spelling
# misspell works with gometalinter
retool add github.com/client9/misspell/cmd/misspell v0.3.4
# goword adds additional capability to checking comments
Copy link
Member

Choose a reason for hiding this comment

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

s/checking/check/?

Makefile
check: check-setup fmt lint vet

# These need to be fixed before they can be ran regularly
check-fail: goword check-static check-slow
Copy link
Member

Choose a reason for hiding this comment

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

Could you file a github issue for these failed checks? Maybe we can make these checks happy again with the help of the community. 😄

Makefile Outdated
# linter
retool add github.com/mgechev/revive 7773f47324c2bf1c8f7a5500aff2b6c01d3ed73b

check-setup:
Copy link
Member

Choose a reason for hiding this comment

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

Should we add tol-install into this section?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

retool sync installs everything from tools.json. tool-install is the record of how that was generated. Its probably better to put tool-install into a hack/scripts/config folder rather than clutter the Makefile.

zz-jason
zz-jason approved these changes Aug 2, 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
Copy link
Member

zz-jason commented Aug 2, 2018

@shenli @coocood PTAL

@zz-jason zz-jason added type/enhancement The issue or PR belongs to an enhancement. status/LGT1 Indicates that a PR has LGTM 1. labels Aug 2, 2018
@gregwebs gregwebs mentioned this pull request Aug 2, 2018
Closed
ngaut
ngaut approved these changes Aug 2, 2018
View reviewed changes
Copy link
Member

@ngaut ngaut left a comment

Choose a reason for hiding this comment

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

LGTM. Close #7247

@gregwebs
Copy link
Contributor Author

gregwebs commented Aug 2, 2018

#7247 was opened as requested by @zz-jason

@ngaut ngaut added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 2, 2018
@ngaut ngaut merged commit 0f4aba1 into master Aug 2, 2018
@ngaut ngaut deleted the gregwebs/retool branch August 2, 2018 13:20
This was referenced Dec 19, 2018
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ngaut ngaut ngaut approved these changes

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

@shenli shenli Awaiting requested review from shenli

Assignees
No one assigned
Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes. status/LGT2 Indicates that a PR has LGTM 2. 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.
4 participants
@gregwebs @zz-jason @lysu @ngaut