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.

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

*: apply golangci-lint to the new code #22824

Merged
merged 14 commits into from
Feb 24, 2021
Merged

Conversation

tangenta
Copy link
Contributor

What problem does this PR solve?

Fix part of #22754.

What is changed and how it works?

Related changes

  • PR to update pingcap/docs/pingcap/docs-cn:
  • Need to cherry-pick to the release branch

Check List

Tests

  • Unit test
  • Integration test

Side effects

N/A

Release note

  • No release note

@tangenta tangenta requested review from a team as code owners February 19, 2021 10:42
@tangenta tangenta requested review from XuHuaiyu and removed request for a team February 19, 2021 10:42
@github-actions github-actions bot added sig/execution SIG execution sig/sql-infra SIG: SQL Infra labels Feb 19, 2021
@bb7133
Copy link
Member

bb7133 commented Feb 20, 2021

As reported by the CI: https://internal.pingcap.net/idc-jenkins/job/tidb_ghpr_check_2/74672/display/redirect

https://github.com/pingcap/tidb/pull/22824/files#diff-15fd19c08250c10f2c8ae4287c8c8d70eaf2b2d103bfed7c13add81053ebffb7R484 Actually returns an 'data truncated' error for ENUM, please update the test case or use MustGetError instead.

@tangenta
Copy link
Contributor Author

This PR increases the make check-static time from 5 seconds to around 30 seconds, I am not sure if this is acceptable.

Copy link
Contributor

@AilinKid AilinKid left a comment

Choose a reason for hiding this comment

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

LGTM

@ti-srebot ti-srebot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 20, 2021
@AilinKid
Copy link
Contributor

This PR increases the make check-static time from 5 seconds to around 30 seconds, I am not sure if this is acceptable.

the 30s seem ok, how do you think it? @bb7133

@bb7133
Copy link
Member

bb7133 commented Feb 20, 2021

There are still some failed cases, please try to figure out the reasons and fix them.

@ngaut
Copy link
Member

ngaut commented Feb 20, 2021

Good job.

@bb7133
Copy link
Member

bb7133 commented Feb 22, 2021

This PR increases the make check-static time from 5 seconds to around 30 seconds, I am not sure if this is acceptable.

the 30s seem ok, how do you think it? @bb7133

I think it's fine since it is checked in check_dev, which is not a costly CI item.

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

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Feb 22, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Feb 22, 2021
@ti-srebot ti-srebot removed the status/LGT2 Indicates that a PR has LGTM 2. label Feb 22, 2021
@jackysp
Copy link
Member

jackysp commented Feb 24, 2021

/lgtm

@jackysp
Copy link
Member

jackysp commented Feb 24, 2021

/merge

@ti-chi-bot
Copy link
Member

@jackysp: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot
Copy link
Member

@jackysp: /merge in this pull request requires 2 /lgtm.

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@bb7133
Copy link
Member

bb7133 commented Feb 24, 2021

/lgtm

@jackysp
Copy link
Member

jackysp commented Feb 24, 2021

/merge

@ti-chi-bot
Copy link
Member

@jackysp: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot
Copy link
Member

@jackysp: /merge in this pull request requires 2 /lgtm.

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@bb7133 bb7133 added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT3 The PR has already had 3 LGTM. labels Feb 24, 2021
@ti-chi-bot ti-chi-bot added status/LGT3 The PR has already had 3 LGTM. and removed status/LGT2 Indicates that a PR has LGTM 2. labels Feb 24, 2021
@bb7133 bb7133 removed the status/LGT3 The PR has already had 3 LGTM. label Feb 24, 2021
@ti-chi-bot ti-chi-bot added the status/LGT3 The PR has already had 3 LGTM. label Feb 24, 2021
@bb7133 bb7133 added the status/LGT2 Indicates that a PR has LGTM 2. label Feb 24, 2021
@ti-chi-bot ti-chi-bot removed the status/LGT2 Indicates that a PR has LGTM 2. label Feb 24, 2021
@bb7133 bb7133 added the require-LGT3 Indicates that the PR requires three LGTM. label Feb 24, 2021
@bb7133
Copy link
Member

bb7133 commented Feb 24, 2021

/merge

@ti-chi-bot
Copy link
Member

@bb7133: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: faf7bad

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 24, 2021
@ti-chi-bot ti-chi-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 24, 2021
@ti-chi-bot
Copy link
Member

@tangenta: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
require-LGT3 Indicates that the PR requires three LGTM. sig/execution SIG execution sig/sql-infra SIG: SQL Infra size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. status/can-merge Indicates a PR has been approved by a committer. status/LGT3 The PR has already had 3 LGTM.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants