Repository navigation
chore(dockerfiles): add Go 1.27.2 CI runtime profile - #5365
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 introduces a new Skaffold profile for Go 1.27.2 alongside the existing Go 1.25 profile, updating GitHub Actions workflows to build and release images for both Go versions on amd64 and arm64 platforms. The approach uses YAML patches in Skaffold config and updates the build matrix accordingly. The changes are well scoped, preserving existing defaults and CI logic. Overall, the PR is clean and follows a safe incremental approach. However, some improvements and verifications can enhance maintainability and robustness.
Critical Issues
- No critical issues identified. The changes are limited to CI and build configuration updates without breaking existing functionality.
Code Improvements
-
Inconsistent patch operation in
skaffold.yaml(lines 37-51):
Thereplaceoperation for the/build/tagPolicy/customTemplate/templatepath assumes that this path always exists. If it does not,replacewill fail. Usingaddorreplaceconditionally or confirming the path exists in the base config is safer.Suggestion:
Confirm the base Skaffold config has this path for thego-1.25profile or consider usingaddwith- op: addif uncertain, or combine with- op: replaceonly after validation. -
Hardcoded Go version in Skaffold profile buildArgs (lines 45-50):
The Go version string1.27.2is hardcoded in both the patch and the Dockerfile build arguments. To reduce duplication and ease future upgrades, consider using a variable or centralized version management.Suggestion:
If possible, define a variable or use a renovate-compatible datasource comment on the version line to automate updates. -
Lack of explicit Skaffold
buildArgsmerge (lines 45-50):
The patch uses- op: addat/build/artifacts/0/docker/buildArgs. If the base profile already has buildArgs, this will cause an error or overwrite. Confirm that the base profile has no existingbuildArgsor use a merge strategy.Suggestion:
Use- op: addwithmergestrategy or verify base config. Alternatively, patch theGOLANG_VERSIONkey inside existing buildArgs rather than replacing the whole object.
Best Practices
-
Missing comments for new Skaffold profile (lines 37-52):
The newgo-1.27profile YAML patch lacks explanatory comments like thego-1.25profile’s# renovate: datasource=docker depName=golangcomment on the Go version. Adding comments improves maintainability and helps automation tools.Suggestion:
Add comments before the profile or buildArgs section to explain purpose and renovate integration. -
Testing coverage note in PR description:
The PR notes that test coverage is "not measured." While understandable for declarative config, consider adding integration or smoke tests that confirm the new Go version profile builds the expected images successfully.Suggestion:
Add a minimal CI job or test to build the new profile image and verify thego-versionlabel or Go runtime version inside the container. -
Workflow matrix duplication (lines 60-65 in both workflow files):
The matrix update is repeated in two similar workflow files. Consider extracting matrix definitions into reusable workflow templates or YAML anchors to reduce duplication and risk of inconsistent updates.Suggestion:
Refactor workflows to use reusable matrices or templates if the project growth justifies it.
Minor style / naming notes
- The
go-profilematrix values usego-1.25andgo-1.27. Consider using a consistent version format likego1.25/go1.27without dashes if that matches other parts of the repo or Docker tags to avoid confusion.
Summary of suggested actions:
- Confirm Skaffold patch `replace` operation path exists or switch to safer patch operation.
- Ensure `buildArgs` patch merges with existing args rather than overwriting.
- Add renovate datasource comments and clarifying comments to new Skaffold profile.
- Add minimal integration tests verifying build and Go version label.
- Consider workflow matrix DRY refactor if applicable.
- Review naming consistency for `go-profile` values.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 introduces a new Skaffold profile go-1.27 to support Go 1.27.2 in CI runtime Docker builds, alongside the existing Go 1.25 profile. It updates CI workflow matrices to build and test both Go versions on amd64 and arm64 platforms. The changes are well-contained to declarative configuration and workflow YAML, preserving backward compatibility and existing defaults. The PR is well-documented with thorough validation steps, though Docker builds were not tested locally. Overall, the changes appear sound and maintainable.
Critical Issues
- Dockerfile MinIO and mc download URLs (dockerfiles/ci/tici/Dockerfile, lines ~9-15)
The PR changes MinIO and mc download URLs fromdl.min.ioto GitHub release assets with a hardcoded future release date (RELEASE.2025-02-07T23-21-09Z).- Issue: This introduces a potential fragility or breakage if those GitHub release URLs are not stable or the release artifacts are not present as expected. It also introduces a hardcoded future timestamp, which may cause build failures if the files are not available yet.
- Recommendation: Confirm if this URL change is intentional and tested. If this is a fix or update unrelated to Go version, consider splitting it out into a separate PR. Otherwise, at minimum, document the rationale and confirm the URLs' availability. Consider adding a retry or checksum verification for downloads to improve reliability.
Code Improvements
-
Skaffold Profile Patch Path (dockerfiles/ci/skaffold.yaml, lines ~38-48)
The profile forgo-1.27patches thetagPolicy.customTemplate.templateand adds build args forGOLANG_VERSION.- Improvement: Consider explicitly replacing the whole
buildArgsmap instead of usingaddwhich may cause merge conflicts if other build args exist. Usingreplacefor the whole buildArgs path ensures the profile is self-contained and avoids unintended inheritance. - Suggested patch snippet:
- op: replace path: /build/artifacts/0/docker/buildArgs value: GOLANG_VERSION: 1.27.2
- Improvement: Consider explicitly replacing the whole
-
Workflow Matrix Consistency (
.github/workflows/pull-ci-runtime-images.yamlandrelease-ci-runtime-images.yaml)
The matrix addsgo-profile: [go-1.25, go-1.27].- Improvement: Verify that all jobs and steps consuming
go-profilehandle multiple profiles correctly, including any conditional logic. Consider adding explicit job names or output annotations reflecting the Go version for easier debugging in CI logs.
- Improvement: Verify that all jobs and steps consuming
Best Practices
-
Documentation
The PR description is detailed and helpful. However, the Skaffold profile addition inskaffold.yamllacks inline comments explaining the patch rationale. Adding comments improves maintainability.- Suggested addition:
- name: go-1.27 # Profile for Go 1.27.2 builds - tags images with go1.27 and sets Go version build arg patches: ...
- Suggested addition:
-
Test Coverage
The PR notes that test coverage is not measured due to declarative config. Consider adding CI steps that run basic smoke tests or build verification on the new Go 1.27 images once published, to catch runtime incompatibilities early. -
Naming Conventions
Thego-profilenamesgo-1.25andgo-1.27follow a consistent pattern. Confirm this naming is aligned with other tooling consuming these labels to avoid confusion.
Overall, this is a well-prepared PR extending CI support for Go 1.27 with minimal risk. Address the noted Dockerfile download URL concerns and consider the suggested improvements for Skaffold patching and documentation to enhance robustness and maintainability.
|
PTAL done — reviewed the full diff. Looks good:
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
go-1.27Skaffold profile for Go 1.27.2, preserving the existinggo-1.25profile and Dockerfile defaults.cimodule buildsbaseand dependentjenkinsimages forlinux/amd64andlinux/arm64, using the existing-go1.27tag convention andgo-versionlabel propagation.ci-ticiMinIO download step by replacing the two retireddl.min.ioURLs with official GitHub release assets, retaining exactly MinIORELEASE.2025-02-07T23-21-09Zand mcRELEASE.2025-02-08T19-14-21Z, both architectures, installed names, and destinations.Validation
build --dry-runfor both Go profiles on both architectures: passed. Workflows retain their existing Skaffold v2.16.0.-shellcheck=; shellcheck is not installed locally).git diff --check: passed.pi-codex-company, code, max 3 rounds, 2 retries): CLEAN in round 1, 53 seconds; separate final verification PASS, 15 seconds.--versionsmoke tests passed (MinIO/mc × Linux amd64/arm64), confirming both retained releases.ci-ticieffective-config check and workflow actionlint passed; read-onlypi-deepseekreview-loop CLEAN in round 1, 55 seconds; separate final verification PASS after all binary/runtime checks completed.Initial CI failure and follow-up
Initial run: https://github.com/PingCAP-QE/ci/actions/runs/38847005508
ci-ticiarm64 failed at MinIO download (wgetexit 8). Both old server/client URLs return HTTP 410 Gone for both architectures; the retired site no longer serves community releases, so retrying alone cannot recover this job. The follow-up changes only the two download URLs, not the MinIO versions or installation policy.archive.apache.orgfor Maven (wgetexit 4, connection timeout). The same Maven download succeeded in the same run's Go 1.27 arm64 job, and the endpoint now returns HTTP 200: no Maven source change is made for this transient failure.Publication boundary
PR checks use
--push=false: this PR does not publish an image or establish a usable downstream tag. After a separately authorized merge and a successful publish run, retrieve the exactghcr.io/pingcap-qe/ci/{base,jenkins}:<tag>-go1.27tags and verify the final manifests include both Linux architectures and the image'sgo-version=1.27.2before updating downstream CI job references.This PR is left unmerged for user review; auto-merge is not enabled.