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

feat(tiflow): add trigger for new sync_diff pipeline #3392

Conversation

purelind
Copy link
Collaborator

@purelind purelind commented Feb 17, 2025

Ref #3387

Add trigger for new pipeline of sync_diff_inspector. Enable the condition trigger and remove the WIP after passing the test.

Copy link

ti-chi-bot bot commented Feb 17, 2025

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

Key Changes:

  1. A new pipeline pull_syncdiff_integration_test has been added to the jobs/pingcap/tiflow/latest/ directory.
  2. Changes have been made to the latest-presubmits.yaml file to add the new pipeline as a presubmit job.

Potential Problems:

  1. In the latest-presubmits.yaml file, the optional: true flag is set for the pull_syncdiff_integration_test job. This might cause the job to be skipped if there are no changes related to it, which could lead to incomplete testing.
  2. The context field in the latest-presubmits.yaml file is set to wip/pull-syncdiff-integration-test. It should be removed after the test is ready, but there is a TODO comment suggesting to remove it. Make sure this is addressed before merging.

Fixing Suggestions:

  1. Remove the optional: true flag from the pull_syncdiff_integration_test job in the latest-presubmits.yaml file to ensure it runs on every change.
  2. Address the TODO comments in the latest-presubmits.yaml file by removing the context field once the test is ready to avoid any confusion or issues in the future.

Overall, the changes look good, but the potential problems mentioned above should be addressed before merging the pull request.

@purelind purelind changed the title feat(tilfow): add trigger for new sync_diff pipeline feat(tiflow): add trigger for new sync_diff pipeline Feb 17, 2025
Copy link

ti-chi-bot bot commented Feb 17, 2025

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

Key Changes:

  1. Added a new pipeline job pull_syncdiff_integration_test in jobs/pingcap/tiflow/latest/pull_syncdiff_integration_test.groovy file.
  2. Updated the latest-presubmits.yaml file to include the new pipeline job pingcap/tiflow/pull_syncdiff_integration_test.

Potential Problems:

  1. The description mentions removing the WIP after passing the test, but it is not clear how the passing criteria will be determined. It's important to have clear criteria for when to remove the WIP.
  2. In the latest-presubmits.yaml file, the optional: true flag is set for the new pipeline job. It's important to ensure that the job is not marked as optional if it is critical for the pipeline.
  3. The skip_if_only_changed field in the latest-presubmits.yaml file is currently commented out. It should be uncommented and properly configured once the test is ready.

Fixing Suggestions:

  1. Add clear criteria in the description or comments of the PR for when the WIP flag should be removed.
  2. Ensure that the optional flag is set appropriately based on the importance of the new pipeline job.
  3. Uncomment and configure the skip_if_only_changed field in the latest-presubmits.yaml file once the test is ready.

Overall, the changes look good, but the potential problems identified should be addressed before merging the PR.

Copy link

ti-chi-bot bot commented Feb 17, 2025

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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 merged commit 43dc4e0 into PingCAP-QE:main Feb 17, 2025
2 checks passed
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.

1 participant