fix(cd): update tikv builder go version to 1.25.10#985
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 downgrades the Go toolchain version from 1.26.1 to 1.25.10 in the TiKV builder Dockerfiles for both the standard and FIPS versions. The change is localized to Dockerfile ARG versions for Go, without modifying any other build or configuration files. The update appears straightforward and low risk, as it only affects the base Go version used during builds.
Code Improvements
- Add checksum verification for downloaded Go tarball
- Files:
dockerfiles/cd/builders/tikv/Dockerfile(line ~24)dockerfiles/cd/builders/tikv/fips.Dockerfile(line ~22)
- Why:
Currently, the Go tarball is downloaded and extracted without verifying its integrity. This can lead to silent failures or security risks if the download is corrupted or tampered with. - Suggestion:
Add a checksum verification step using a known SHA256 sum for the Go version being installed. For example:This ensures the downloaded file matches the official checksum.ARG GOLANG_VERSION=1.25.10 ARG GOLANG_SHA256=<expected-sha256-sum> RUN OS=linux; ARCH=$([ "$(arch)" = "x86_64" ] && echo amd64 || echo arm64); \ curl -fsSL https://dl.google.com/go/go${GOLANG_VERSION}.linux-${ARCH}.tar.gz -o go.tar.gz && \ echo "${GOLANG_SHA256} go.tar.gz" | sha256sum -c - && \ tar -C /usr/local -xzf go.tar.gz && rm go.tar.gz
- Files:
Best Practices
-
Clarify reason for downgrade in PR description
- File: PR description
- Why:
Downgrading the Go version can have implications on compatibility, security, or build stability. The PR description lacks context on why this downgrade is necessary. - Suggestion:
Add a brief explanation about the rationale behind downgrading (e.g., compatibility issues with Go 1.26.1, stability concerns, or known regressions). This helps reviewers and future maintainers understand the motivation.
-
Document the Go version ARG in Dockerfile comments
- Files:
dockerfiles/cd/builders/tikv/Dockerfile(line ~20)dockerfiles/cd/builders/tikv/fips.Dockerfile(line ~18)
- Why:
The ARG for Go version is critical for build reproducibility. Adding a comment explaining the purpose of this ARG and where to find compatible versions helps maintainers update it in the future. - Suggestion:
Add a comment like:# GOLANG_VERSION controls the Go toolchain version used for building TiKV. # Update with care; ensure compatibility with TiKV build requirements. ARG GOLANG_VERSION=1.25.10
- Files:
Testing Coverage
- Consider minimal build verification after change
- File: PR description / testing section
- Why:
Although this is an ARG-only change, sometimes Go version changes can cause subtle build or dependency resolution issues. A minimal build test or smoke test would increase confidence in the change. - Suggestion:
Add a note in the PR or pipeline to run a quick build to verify the new Go version works as expected, even if manual testing is minimal.
No critical issues or broken functionality identified given the limited scope of the change. Implementing checksum verification and adding rationale/context will improve security and maintainability.
There was a problem hiding this comment.
Code Review
This pull request updates the GOLANG_VERSION argument from 1.26.1 to 1.25.10 in both dockerfiles/cd/builders/tikv/Dockerfile and dockerfiles/cd/builders/tikv/fips.Dockerfile. Since the review comments pertain to version numbers (which are non-code changes), they have been filtered out, and there is no additional feedback to provide.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
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 downgrades the Go toolchain version from 1.26.1 to 1.25.10 in the RockyLinux Tikv builder and its FIPS variant Dockerfiles. The approach is a straightforward ARG value change in both Dockerfiles with no other modifications. The update is simple and low risk given it only affects the build environment version specification, but no tests were run since it is an ARG-only change.
Code Improvements
-
Dockerfiles:
dockerfiles/cd/builders/tikv/Dockerfileanddockerfiles/cd/builders/tikv/fips.Dockerfile(lines ~21 and ~19 respectively)- Currently the Go version is hardcoded via an ARG, which is fine, but consider adding a fallback or validation step to ensure the specified Go version is actually available in the remote URL before downloading. This could prevent build failures if the version is removed or renamed on the Go download server.
- Example snippet after the
curlcommand:This makes it clearer and easier to add error handling or caching in the future.RUN curl -fsSL https://dl.google.com/go/go${GOLANG_VERSION}.linux-${ARCH}.tar.gz -o go.tar.gz && \ tar -C /usr/local -xzf go.tar.gz && \ rm go.tar.gz
-
Edge case consideration:
- The PR does not mention if the downgrade impacts any downstream dependencies or builds. It’s recommended to document or verify if Go 1.25.10 is compatible with all Tikv build scripts or toolchains that will use this builder image.
Best Practices
-
Testing coverage gap:
- Although this is a simple ARG change, it is best practice to run a minimal validation build to ensure the Docker image builds successfully with the downgraded Go version. This prevents surprises downstream.
-
Documentation:
- The PR or the Dockerfile could benefit from a brief comment explaining the reason for the downgrade (e.g., compatibility, stability, security, or known issues with 1.26.1). This helps future maintainers understand context.
-
Style / Consistency:
- The existing comment
# renovate: datasource=docker depName=golangis good for automated tooling. Ensure that any related tooling or documentation is updated to reflect the version change if applicable.
- The existing comment
No critical issues found given the limited scope and nature of the change. The PR is safe but would benefit from minimal testing and improved documentation for clarity and maintainability.
Summary
Testing