ci icon indicating copy to clipboard operation
ci copied to clipboard

feat(tiflash): install clang-format-17 if not exist

Open Lloyd-Pottiger opened this issue 10 months ago • 1 comments

Lloyd-Pottiger avatar Apr 10 '24 10:04 Lloyd-Pottiger

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

This pull request seems to be about changing the version of clang-format used in the tiflash pipeline from version 12 or 15 to version 17. The pull request also updates the docker image used in the pipeline.

Key changes:

  1. The docker image was changed from hub.pingcap.net/tiflash/tiflash-llvm13-amd64:v20231214 to hub.pingcap.net/tiflash/tiflash-llvm-base:amd64-llvm-17.0.6 in several files.

  2. The Prepare tools stage that was installing several tools like ccache, cmake3, clang-format, clang-format-15, clang-tidy, and gcovr, was completely removed from the pipeline in several groovy files.

Potential problems:

  1. The pull request is missing a detailed description. It would be helpful to provide a rationale for these changes.

  2. By removing the Prepare tools stage, the pipeline now assumes that all required tools are pre-installed in the new docker image. If this is not the case, the pipeline could fail.

  3. The change in clang-format version may have implications on the codebase if the new version introduces any breaking changes or behaves differently from the previous version.

Fixing suggestions:

  1. Add a detailed description explaining the reasons for the changes, the impact, and the benefits.

  2. Ensure that all the tools previously installed in the Prepare tools stage are indeed present in the new Docker image.

  3. Test the pipeline with the new clang-format version to make sure it behaves as expected.

ti-chi-bot[bot] avatar Jun 25 '24 06:06 ti-chi-bot[bot]

/hold

purelind avatar Jun 25 '24 06:06 purelind

[LGTM Timeline notifier]

Timeline:

  • 2024-06-25 06:32:29.121395787 +0000 UTC m=+701275.606884621: :ballot_box_with_check: agreed by purelind.

ti-chi-bot[bot] avatar Jun 25 '24 06:06 ti-chi-bot[bot]

/cc @JaySon-Huang

purelind avatar Jun 25 '24 06:06 purelind

@JaySon-Huang: 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[bot] avatar Jun 25 '24 06:06 ti-chi-bot[bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JaySon-Huang, purelind

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[bot] avatar Jun 25 '24 06:06 ti-chi-bot[bot]

/unhold

purelind avatar Jun 25 '24 06:06 purelind