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

chore(pingcap/tidb): update trigger for CI job pull-br-integration-test #3040

Merged
merged 2 commits into from
Jul 31, 2024

Conversation

wuhuizuo
Copy link
Collaborator

Also triggerBR integration testing when changes are made to DDL module. @BornChanger @Benjamin2037

Fixes #3039

Signed-off-by: wuhuizuo wuhuizuo@126.com

…est`

Also trigger`BR` integration testing when changes are made to `DDL` module. @BornChanger @Benjamin2037

Fixes #3039

Signed-off-by: wuhuizuo <wuhuizuo@126.com>
@ti-chi-bot ti-chi-bot bot requested a review from purelind July 31, 2024 07:20
Copy link

ti-chi-bot bot commented Jul 31, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Based on the title and description, it seems that the PR is making a change to the trigger for the CI job pull-br-integration-test in the pingcap/tidb repository, specifically to include changes to the DDL module.

The diff shows that the run_if_changed condition for the job has been updated to (br|pkg/ddl)/.*, which should trigger the job for changes to either the br or pkg/ddl directories.

One potential problem with this change is that it may cause the CI job to run unnecessarily for changes that are not actually relevant to the DDL module. It's important to carefully consider the impact of widening the trigger condition.

A possible suggestion to mitigate this issue is to also add a check within the CI job that verifies whether the changes actually affect the DDL module before running the integration tests. This can ensure that the job only runs when necessary.

Overall, the change seems reasonable and appropriate as long as the potential impact is carefully considered.

@wuhuizuo
Copy link
Collaborator Author

/cc @BornChanger @Benjamin2037 @D3Hunter
PTAL

Copy link

ti-chi-bot bot commented Jul 31, 2024

I have already done a preliminary review for you, and I hope to help you do a better job.

Based on the PR title and description, it seems that the change is related to updating the trigger for a CI job in the pingcap/tidb repository. Specifically, the change will now trigger the BR integration testing whenever changes are made to the DDL module.

In terms of potential problems, it's hard to identify any major issues based on the provided information. However, it's always important to consider the impact of changes to CI/CD pipelines, as any errors or issues with the pipeline could potentially impact the development process.

Regarding fixing suggestions, one recommendation would be to thoroughly test the updated trigger in a staging environment before merging the changes into the main branch. Additionally, it might be helpful to seek feedback from other members of the development team to ensure that the updated trigger doesn't negatively impact their workflows.

@wuhuizuo wuhuizuo added the lgtm label Jul 31, 2024
@wuhuizuo
Copy link
Collaborator Author

/approve

Copy link

ti-chi-bot bot commented Jul 31, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wuhuizuo

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 Jul 31, 2024
@ti-chi-bot ti-chi-bot bot merged commit 6442721 into main Jul 31, 2024
2 checks passed
@ti-chi-bot ti-chi-bot bot deleted the wuhuizuo/issue3039 branch July 31, 2024 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

also triggerBR integration testing when changes are made to DDL module. @BornChanger @Benjamin2037
2 participants