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

*: enable lint test. #11647

Merged
merged 1 commit into from
Oct 12, 2024
Merged

Conversation

Benjamin2037
Copy link
Contributor

@Benjamin2037 Benjamin2037 commented Oct 10, 2024

What problem does this PR solve?

Issue Number: ref #11606

What is changed and how it works?

enable lint checks that was temp disable due to upgrade go to 1.23.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test (add detailed scripts or steps below)
    [x] No code

Questions

Will it cause performance regression or break compatibility?
Do you need to update user documentation, design documentation or monitoring documentation?

Release note

N/A

@ti-chi-bot ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. area/dm Issues or PRs related to DM. area/engine Issues or PRs related to Dataflow Engine. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Oct 10, 2024
Copy link

codecov bot commented Oct 10, 2024

Codecov Report

Attention: Patch coverage is 52.05479% with 35 lines in your changes missing coverage. Please review.

Project coverage is 55.3246%. Comparing base (8532fab) to head (2acc977).
Report is 4 commits behind head on master.

Additional details and impacted files
Components Coverage Δ
cdc 59.9579% <31.8181%> (+0.0926%) ⬆️
dm 50.0914% <60.0000%> (+0.0269%) ⬆️
engine 53.2392% <100.0000%> (+0.0139%) ⬆️
Flag Coverage Δ
unit 55.3246% <52.0547%> (+0.0610%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@@               Coverage Diff                @@
##             master     #11647        +/-   ##
================================================
+ Coverage   55.2635%   55.3246%   +0.0610%     
================================================
  Files          1001       1001                
  Lines        136162     136310       +148     
================================================
+ Hits          75248      75413       +165     
+ Misses        55426      55409        -17     
  Partials       5488       5488                

Copy link
Contributor

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

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

we can see only dm uses "github.com/pingcap/check". we should replace it with testify gradually. pingcap/tidb#26022

@ti-chi-bot ti-chi-bot bot added the needs-1-more-lgtm Indicates a PR needs 1 more LGTM. label Oct 10, 2024
@ti-chi-bot ti-chi-bot bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels Oct 11, 2024
Copy link
Contributor

ti-chi-bot bot commented Oct 11, 2024

[LGTM Timeline notifier]

Timeline:

  • 2024-10-10 11:19:32.394242478 +0000 UTC m=+1131327.814455489: ☑️ agreed by lance6716.
  • 2024-10-11 05:32:31.795085875 +0000 UTC m=+1196907.215298887: ☑️ agreed by 3AceShowHand.

@3AceShowHand
Copy link
Contributor

the cdc part LGTM

Copy link
Contributor

@wlwilliamx wlwilliamx left a comment

Choose a reason for hiding this comment

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

cdc part LGTM

@@ -615,6 +615,7 @@ func processEvent(
return nil, event, nil
}
}
// nolint
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please explain why the // nolint comment was added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This nolint is used for revive check,if last else with return or break,suggest to remove else part.

@@ -99,6 +99,7 @@ func codecEncodeKeyPB(event *model.RowChangedEvent) []byte {
RowId: event.RowID,
Partition: 0,
}
// nolint
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an issue with the current linting for panic, which is why the // nolint comment was added here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is also revive check, suggest to remove else part of if statement.

Copy link
Contributor

ti-chi-bot bot commented Oct 11, 2024

@wlwilliamx: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

cdc part LGTM

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 kubernetes/test-infra repository.

Copy link
Collaborator

@purelind purelind left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link
Contributor

ti-chi-bot bot commented Oct 11, 2024

@purelind: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

/lgtm

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 kubernetes/test-infra repository.

@ti-chi-bot ti-chi-bot bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 11, 2024
Copy link

@yudongusa yudongusa left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

ti-chi-bot bot commented Oct 11, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 3AceShowHand, lance6716, purelind, wlwilliamx, yudongusa

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot added the approved label Oct 11, 2024
@Benjamin2037
Copy link
Contributor Author

/test cdc-integration-storage-test

@ti-chi-bot ti-chi-bot bot merged commit f6b41fe into pingcap:master Oct 12, 2024
28 checks passed
@Benjamin2037 Benjamin2037 deleted the enable-lint-check branch October 12, 2024 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved area/dm Issues or PRs related to DM. area/engine Issues or PRs related to Dataflow Engine. lgtm release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants