Repository navigation
feat(dockerfiles): add Go 1.27.2 builder images - #1113
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 updates the continuous delivery (CD) builder pipeline to support Go version 1.27.2 while retaining compatibility with prior Go versions (1.26 and 1.25). The changes involve adding new profiles to Skaffold configurations, modifying Dockerfiles to default to 1.27.2, and updating relevant GitHub workflows to include the new Go version. The approach is thorough, following established conventions for profiles, tags, and environments. The validation checklist and metadata suggest high attention to detail, though the lack of full local builds is noted. Overall, the changes appear well-structured and maintain backward compatibility without introducing major risks.
Critical Issues
No critical issues detected.
Code Improvements
-
Repeated ARG declaration in
dockerfiles/cd/builders/tidb-operator/Dockerfile:- File:
dockerfiles/cd/builders/tidb-operator/Dockerfile, Lines 8–9 - Issue:
ARG GOLANG_VERSIONis declared twice in this Dockerfile (before and after FROM) which is redundant and unnecessary. - Suggested Solution: Remove the duplicate declaration before
FROM golang:${GOLANG_VERSION}.# Remove this line: ARG GOLANG_VERSION=1.27.2
- File:
-
Performance bottleneck in full image builds:
- File: Not specific to a file, but mentioned in the PR description under "Validation."
- Issue: The
Rocky Linux dnf package installationduring arm64 builds hits a 360-second timeout. This could lead to CI failures or deployment delays. - Suggested Solution: Investigate and optimize the
dnfpackage installation commands or use a local cache to speed up installation. Alternatively, consider increasing the timeout in CI/CD for this specific build step.
Best Practices
-
Testing coverage gaps:
- File: Multiple Skaffold configurations (e.g.,
dockerfiles/cd/builders/ng-monitoring/skaffold.centos7.yaml, etc.) - Issue: No runtime validation or integration testing is included in the PR, especially for the newly added Go 1.27 profiles. While dry-run checks are helpful, full builds and compatibility tests would ensure the configurations work as expected.
- Suggested Solution: Include a CI step to perform actual builds and runtime tests for the new profiles. You could add a matrix job targeting Go 1.27 builds to validate functionality beyond dry-run checks.
- File: Multiple Skaffold configurations (e.g.,
-
Documentation for builder changes:
- File: Multiple Dockerfiles and Skaffold YAML files.
- Issue: While the PR description is detailed, the individual files lack inline comments explaining the purpose of the new Go 1.27 profiles or environment updates.
- Suggested Solution: Add comments in each modified Dockerfile and Skaffold configuration to clarify the context and rationale for adding
go-1.27profiles. Example:# Added go-1.27 profile for compatibility with Go 1.27.2 toolchain. Retains backward compatibility with Go 1.26 and 1.25.
-
Style guideline deviations in Dockerfiles:
- File: Multiple Dockerfiles (e.g.,
dockerfiles/cd/builders/ng-monitoring/Dockerfile) - Issue: The
curlcommands lack retries or verification flags, which can lead to unstable builds if the tarball download fails or is corrupted. - Suggested Solution: Use
--retryand--retry-connrefusedflags, and validate the checksum of the downloaded file. Example:RUN OS=linux; ARCH=$([ "$(arch)" = "x86_64" ] && echo amd64 || echo arm64); \ curl --retry 5 --retry-connrefused -fsSL https://dl.google.com/go/go${GOLANG_VERSION}.linux-${ARCH}.tar.gz | \ tar -C /usr/local -xz
- File: Multiple Dockerfiles (e.g.,
Conclusion
The PR is generally clean and well-structured, with no major issues. Addressing the redundant ARG declaration, optimizing the dnf installation, enhancing test coverage, and improving comments/documentation will make this update more robust and maintainable.
|
PTAL done — reviewed the full diff against the existing
One non-blocking follow-up (pre-existing, not introduced by this PR): PD's Dockerfiles have never been included in Renovate's Go patch PRs — they were stuck at Thanks @dillon-zheng — LGTM 👍 |
|
[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 |
[LGTM Timeline notifier]Timeline:
|
Summary
Prepare Go 1.27.2 CD builder images without switching any package routes or image consumers.
go-1.27profiles to all 30 existing Go-builder Skaffold variants, preservinggo-1.26/go-1.25, Renovate annotations, tag conventions, and amd64 + arm64 platforms.1.27.2.go-1.27to the Go matrix in all four pull/publish workflows; both root and non-root builds use these matrices.ARG GOLANG_VERSIONinside thetidb-operatorbuilder stage so its existinggo-versionlabel receives the selected version.Rust/TiKV and C++ builders, existing Go profiles, yum source/verification policy, package routing, generated release YAML, and CI job image references are unchanged. The unrelated pre-existing C++ matrix in the CentOS 7 pull workflow is intentionally unchanged. Original PD Dockerfile CRLF endings are preserved.
Validation
skaffold build --dry-run --push=falsetag checks.-shellcheck=; no shell script changes).--checkfor the operator builder withGOLANG_VERSION=1.27.2: no warnings.git -c core.whitespace=blank-at-eol,blank-at-eof,space-before-tab,cr-at-eol diff --check.Runtime line coverage: not measured (Docker/Skaffold/workflow configuration change). Configuration checks cover every changed profile and workflow.
Change review
Reviewed using
pi-deepseekthrough review-loop (read-only reviewer, Codex author/fixer): round 1VERDICT: CLEAN, no Critical/Warning/Suggestion findings, 1 minute 26 seconds. No fixer/adjudication step was needed. Branch created from and rechecked/rebased against latestupstream/mainbefore submission.Publication boundary
This PR does not publish images or change routing. PR image checks use
--push=false; publication runs after a separately authorized merge tomain. Exact tags must be read from successful publish runs, then verified as manifest lists containinglinux/amd64+linux/arm64, withgo-version=1.27.2on each architecture. No published Go 1.27 tag is claimed here.Known existing CentOS 7 vault/arm64 failures may still block build/publication; this change does not weaken repository verification or replace yum sources.
Leave unmerged for user review.