fix(packages): update TikV builder image version in YAML template#954
fix(packages): update TikV builder image version in YAML template#954
Conversation
bump builder image to utilize golang toolchain
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 updates the TiKV builder image version in the packages.yaml.tmpl file to a newer tag (v2026.3.29-23-g4eccc5d-centos7-devtoolset) from the previous one. The change is straightforward and limited to a single line in the YAML template, aiming to utilize a newer Go toolchain as described in the PR. The update is minimal and low-risk, and the code quality is good given the nature of the change.
Code Improvements
-
Line 3007 in
packages.yaml.tmpl:
The new image tag is hardcoded and tightly coupled to the release profile condition. This is fine for now, but consider externalizing or templating the image version/tag itself in the template variables or a config file to facilitate easier future updates without modifying the template directly. For example:image: "ghcr.io/pingcap-qe/cd/builders/tikv:{{ .BuilderImageTag }}-centos7-devtoolset{{ ternary "10" "8" (eq .Release.profile "nextgen") }}"
This would make bumping versions easier and reduce potential errors in manual updates.
Best Practices
-
Lack of context or comment on the image bump (around line 3006):
Since image tags can be cryptic, adding a short comment indicating what the new version includes (e.g., "updated Go toolchain version for build optimizations") would improve maintainability and clarity for future reviewers.Suggested comment addition:
# Updated TiKV builder image to v2026.3.29 for newer Go toolchain support -
Testing Validation:
The PR does not mention whether this new builder image has been tested in the build pipeline or with downstream components. Consider adding or documenting testing steps (e.g., CI logs, manual build confirmation) to ensure the new image works correctly.
No critical issues found given the limited scope of the change. The main suggestions focus on maintainability and clarity for future updates.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dillon-zheng 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 |
[LGTM Timeline notifier]Timeline:
|
There was a problem hiding this comment.
Code Review
This pull request updates the TiKV builder image version in packages/packages.yaml.tmpl. Review feedback identifies several critical issues: the FIPS builder image was not updated to match, overlapping version constraints may cause the wrong image to be selected for FIPS builds, and there are inconsistencies between the image tags and the underlying toolchain/OS versions.
| builders: | ||
| - if: {{ semver.CheckConstraint ">= 6.1.0-0" .Release.version }} | ||
| image: "ghcr.io/pingcap-qe/cd/builders/tikv:v2025.4.24-10-g9c32f77-centos7-devtoolset{{ ternary "10" "8" (eq .Release.profile "nextgen") }}" | ||
| image: "ghcr.io/pingcap-qe/cd/builders/tikv:v2026.3.29-23-g4eccc5d-centos7-devtoolset{{ ternary "10" "8" (eq .Release.profile "nextgen") }}" |
There was a problem hiding this comment.
While updating the TikV builder image version, there are several issues that should be addressed:
- FIPS Builder Oversight: The FIPS builder image at line 3011 was not updated to the
v2026.3.29version. Since thefips.Dockerfilealso specifies the new Go toolchain (v1.26.1), this image should be bumped to maintain consistency across build profiles. - Overlapping Constraints: The general builder condition at line 3008 (
>= 6.1.0-0) overlaps with the FIPS-specific condition at line 3010 (~6.5.6-0). If the template engine follows a 'first-match' logic (as suggested by the mutually exclusive ranges used in other components like PD or TiDB), FIPS builds for version 6.5.6 will incorrectly use the non-FIPS image. Consider moving the FIPS entry above the general entry. - Toolchain and OS Inconsistency: The
tikvDockerfile uses Go1.26.1, while other components (PD, TiDB) are using1.25. Additionally, thecentos7-devtoolsetsuffix in the tag is inconsistent with therockylinux:8base used in the Dockerfile provided in the context.
|
no needed, ref: tikv/tikv#19493 |
bump builder image to utilize golang toolchain