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(tidb): enable ddlv1 ut pipeline #3172

Merged

Conversation

purelind
Copy link
Collaborator

@purelind purelind commented Oct 11, 2024

After a period of grayscale testing, we plan to enable this pipeline. This pipeline is set with conditional triggers, and changes to files in the specified directory will trigger it.

@ti-chi-bot ti-chi-bot bot requested a review from wuhuizuo October 11, 2024 03:02
Copy link

ti-chi-bot bot commented Oct 11, 2024

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

Based on the pull request title "feat(tidb): enable ddlv1 ut pipeline", it seems that the changes made are to enable the testing pipeline for ddlv1 in the TIDB project. The diff shows that the changes are made to the latest-presubmits.yaml file in the prow-jobs/pingcap/tidb directory. The changes include modifying the triggers and context for the pingcap/tidb/pull_unit_test_ddlv1 job.

Potential problems:

  • It is not clear what grayscale testing means or what the results were, so it may be beneficial to have more information on that.
  • The reason for enabling this pipeline is not mentioned, which may lead to confusion for other team members.
  • It is not clear if there were any failures or issues during the previous testing that should be addressed before enabling this pipeline.

Fixing suggestions:

  • Provide more information on what the grayscale testing was, and what the results were.
  • Mention the reason for enabling this pipeline, so that other team members are aware of the purpose.
  • Ensure that any failures or issues encountered during previous testing have been addressed before enabling the pipeline.

@ti-chi-bot ti-chi-bot bot added the size/XS label Oct 11, 2024
@purelind
Copy link
Collaborator Author

/hold

Need more approval from the relevant team members.

@purelind
Copy link
Collaborator Author

Copy link

ti-chi-bot bot commented Oct 11, 2024

@purelind: GitHub didn't allow me to request PR reviews from the following users: fixdb.

Note that only PingCAP-QE members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @Benjamin2037 @bb7133 @zanmato1984 @fixdb @D3Hunter

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.

@purelind
Copy link
Collaborator Author

image

@D3Hunter
Copy link
Contributor

background: we are refactoring DDL job args to be a structured struct, we difference this with previous by a different version, but for compatibility we still need to support V1 for a couple of LTS versions, so we want to run unit tests for both versions, to find bugs earlier

Co-authored-by: D3Hunter <jujj603@gmail.com>
Copy link

ti-chi-bot bot commented Oct 11, 2024

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

Based on the pull request title and description, the key change being made is the enabling of the ddlv1 ut pipeline, which has been tested in grayscale. Additionally, the pipeline has been set with conditional triggers, so changes to files in a specified directory will trigger it.

Looking at the diff, it appears that the changes being made are to the latest-presubmits.yaml file for the pingcap/tidb repository. Specifically, the pingcap/tidb/pull_unit_test_ddlv1 job is being modified with the following changes:

  • decorate is being set to false
  • always_run is being set to false
  • optional is being removed
  • run_if_changed is being set to "pkg/(ddl|meta)/.*"
  • context is being changed to pull-unit-test-ddlv1
  • The skip_if_only_changed line is being commented out

It seems that the changes to the job are aimed at making it more efficient and effective by only running it when necessary, based on changes to relevant files.

Potential problems could arise if the conditional triggers are not set up correctly, leading to the job not running when it should or running when it shouldn't. It's also possible that the changes being made could have unintended consequences on other jobs or processes.

One suggestion for fixing would be to thoroughly test the changes in a staging environment before merging the pull request. Additionally, it would be helpful to have clear documentation on the conditional triggers and how they are set up to avoid confusion or mistakes in the future.

Copy link

ti-chi-bot bot commented Oct 11, 2024

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

In response to this:

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

@Benjamin2037 Benjamin2037 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

ti-chi-bot bot commented Oct 12, 2024

@Benjamin2037: 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.

Copy link

ti-chi-bot bot commented Oct 12, 2024

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: Benjamin2037, D3Hunter

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

@purelind
Copy link
Collaborator Author

/unhold

@ti-chi-bot ti-chi-bot bot merged commit aa1c534 into PingCAP-QE:main Oct 12, 2024
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.

3 participants