refactor(packages): update nextgen tiflash builder#989
Conversation
There was a problem hiding this comment.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary:
This PR refactors the packages.yaml.tmpl to update the TiFlash builder images based on the release version and profile conditions. It introduces a new conditional builder for "nextgen" or "experiment" profiles starting from version 8.5.0, while preserving the existing builder for other profiles from 8.2.0 onwards. The change is clear and logically segmented, improving the build matrix with more precise conditions. Overall, the patch is concise and mostly well-structured.
Code Improvements:
-
Complex conditional expressions (packages/packages.yaml.tmpl, lines ~1877-1882):
The new conditional logic uses nestedandandhasfunctions inside theifstatement, which can be hard to read and maintain. Consider extracting these conditions into named template variables or helper functions to improve readability and maintainability, for example:{{ $isNextGen := has (coll.Slice "nextgen" "experiment") .Release.profile }} {{ $versionGTE8_5 := semver.CheckConstraint ">= 8.5.0-0" .Release.version }} builders: - if: {{ and $versionGTE8_5 $isNextGen }} image: ghcr.io/pingcap-qe/cd/builders/tiflash:v2025.4.15-rocky8-llvm-17.0.6-v2 - if: {{ and (semver.CheckConstraint ">= 8.2.0-0" .Release.version) (not $isNextGen) }} image: ghcr.io/pingcap-qe/cd/builders/tiflash:v2025.8.10-2-gc9e3144-centos7
This approach improves clarity and reduces the chance of subtle logic bugs.
-
Version boundary consistency:
The newnextgenprofile builder is gated at>= 8.5.0-0, while the fallback builder remains at>= 8.2.0-0. Ensure this version split was intentional and documented somewhere, as it might cause confusion or unexpected builder selections for versions between 8.2.0 and 8.5.0 with anextgenprofile.
Best Practices:
-
Lack of PR description:
The PR description is empty. Please add a short summary explaining the motivation behind this refactor, especially the introduction of the "nextgen" and "experiment" profile conditions and their impact on the build process. This helps reviewers and future maintainers understand the rationale and context of the change. -
Testing coverage:
There is no indication of tests verifying these conditional branches. Please ensure that the build matrix or CI configuration is tested with release versions and profiles covering:- Version >= 8.5.0 with "nextgen" or "experiment" profiles
- Version >= 8.2.0 without those profiles
- Version < 8.2.0 (existing behavior)
Adding automated tests or at least documented manual test instructions would improve confidence in this change.
No critical issues detected, but improving readability and adding documentation/testing will make the refactor more robust and maintainable.
|
@JaySon-Huang: adding LGTM is restricted to approvers and reviewers in OWNERS files. DetailsIn 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-sigs/prow repository. |
|
related changes: pingcap/tiflash#10887 |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JaySon-Huang, wuhuizuo The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
No description provided.