chore: upgrade tidb CI image for go1.25 #3999
Conversation
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![ti-chi-bot[bot]](https://avatars.githubusercontent.com/in/214286?s=60&v=4){kind=small}
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 pull request upgrades the Go version used in various CI pipeline configurations from go1.23 to go1.25 by updating the container image references across multiple YAML and Groovy files. The approach is straightforward: replacing the image tag in all relevant files. While the changes are mechanical and low-risk, the description lacks context on why the upgrade is necessary, such as compatibility improvements or resolving specific issues in the CI environment. The PR is generally well-executed but could benefit from enhanced documentation and validation steps.
Critical Issues
None Identified
Code Improvements
- Context for Upgrading Go Version:
- Files: All modified files
- Issue: The PR description does not explain why the upgrade to
go1.25is required. This lack of context might confuse reviewers or future contributors who need to audit the change. - Suggestion: Update the PR description to include the rationale for upgrading the Go version (e.g., new features, bug fixes, security patches, or compatibility improvements).
Best Practices
-
Missing Validation of New Image:
- Files: All modified files
- Issue: The PR does not mention whether the new image (
tidb_image:go12520261210) has been tested or validated to ensure compatibility with the current CI pipelines. - Suggestion: Add a note in the PR description or comments indicating that the new image has been verified and tested. If this hasn't been done, ensure that tests are run to confirm compatibility.
-
Documentation for Future Updates:
- Files:
pipelines/pingcap/tidb/latest/pod-ghpr_unit_test.yaml,pipelines/pingcap/tidb/latest/pod-merged_unit_test.yaml, and similar - Issue: Several files contain TODO comments about switching to a standard Bazel build image. This PR does not address these comments, leaving technical debt untouched.
- Suggestion: Evaluate whether these TODOs can be resolved within this or a future PR. If resolving them is out of scope, ensure they are tracked in an issue or backlog.
- Files:
-
Testing Coverage Details:
- Files: All modified files
- Issue: There is no indication of how these changes impact CI workflows or test coverage.
- Suggestion: Ensure that pipeline tests are re-run after merging the PR to verify the functionality of CI pipelines using the new Go image.
Suggested Description Update
This PR updates the Go version used in multiple CI pipeline configurations from `go1.23` to `go1.25`. The change involves replacing the container image tag across all relevant YAML and Groovy files. The upgrade is aimed at leveraging the latest features, performance improvements, and bug fixes in Go 1.25. Validation of the new image should ensure compatibility across all CI workflows.
Summary of ChangesHello @hawkingrei, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request performs a routine but essential infrastructure update by upgrading the continuous integration (CI) environment for TiDB. It systematically updates the Docker image reference across numerous pipeline configuration files to a newer version that incorporates Go 1.25. This ensures that all TiDB builds, checks, and tests are executed against an up-to-date Go runtime, which is crucial for maintaining compatibility, leveraging performance enhancements, and preparing for future Go language features. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request upgrades the TiDB CI image to a version supporting Go 1.25 by updating the image tag across numerous pipeline configuration files. While the update is necessary, the implementation reveals two key areas for improvement. Firstly, the new image is hosted in a personal namespace, which poses potential security and reliability risks for the CI pipeline. It is strongly recommended to use images from an official, organization-managed repository. Secondly, the image tag is hardcoded and duplicated in over 20 files, which makes maintenance difficult. Centralizing this configuration, for example by using variables or templates, would significantly improve maintainability. My review comments provide more details on these points.
| containers: | ||
| - name: golang | ||
| image: "hub.pingcap.net/wangweizhen/tidb_image:go12320241009" | ||
| image: "hub.pingcap.net/wangweizhen/tidb_image:go12520261210" |
There was a problem hiding this comment.
This image definition raises two concerns:
- Use of personal image: The image
hub.pingcap.net/wangweizhen/tidb_image:go12520261210is from a personal namespace (wangweizhen). For security and reliability, production CI pipelines should use images from an official, organization-managed repository. This ensures proper image provenance, security scanning, and availability. - Hardcoded value: The image tag is hardcoded here and duplicated across many other files. This makes future updates difficult and error-prone.
I recommend moving the image to an official repository and defining the image name as a variable within the script to be reused. For example:
final TIDB_CI_IMAGE = "hub.pingcap.net/official-repo/tidb_image:go1.25-latest" // Example official image
final podYaml = """
...
containers:
- name: golang
image: "${TIDB_CI_IMAGE}"
...
"""| containers: | ||
| - name: golang | ||
| image: "hub.pingcap.net/wangweizhen/tidb_image:go12320241009" | ||
| image: "hub.pingcap.net/wangweizhen/tidb_image:go12520261210" |
There was a problem hiding this comment.
This image definition has two issues:
- Use of personal image: The image is from a personal namespace (
wangweizhen). For production CI, images should be sourced from an official, organization-managed repository to guarantee security, provenance, and availability. - Hardcoded value: The image tag is hardcoded and duplicated across numerous configuration files. This makes maintenance difficult and error-prone.
To improve this, the image should be moved to an official repository. Furthermore, the image tag should be parameterized instead of being hardcoded. This can be achieved through templating (e.g., Helm, Kustomize) or by using environment variable substitution in the Jenkins pipeline that processes this YAML file.
Signed-off-by: Weizhen Wang <wangweizhen@pingcap.com>
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 reverts the previous upgrade of the TiDB CI image from Go 1.23 to Go 1.25 by changing the container image tags in multiple Kubernetes pod spec files. The approach is a straightforward replacement of the image tag strings, reverting them back to the prior Go version. The overall changes are mechanical and consistent across files, reducing risk of breaking unrelated functionality. However, the PR does not explain why the revert is needed, which diminishes clarity.
Code Improvements
-
Lack of explanation for revert
- Files: All modified YAML and Groovy files (e.g.,
jenkins/pipelines/ci/tidb/tidb_ghpr_coverage.groovy,pipelines/pingcap/tidb/latest/*.yaml) - Issue: The PR description says it reverts a previous PR but does not explain the reason for reverting the Go 1.25 upgrade (e.g., build failures, incompatibilities, or performance issues). This lack of context may confuse reviewers or future maintainers.
- Suggestion: Add a brief note in the PR description or commit message explaining why the revert is necessary to improve traceability and maintainability. For example:
Revert Go 1.25 upgrade due to [specific issue], switching back to Go 1.23 images until resolved.
- Files: All modified YAML and Groovy files (e.g.,
-
Image tag hardcoding repeated across multiple files
- Files: Multiple pod spec YAML files and one Groovy pipeline file
- Issue: The Go image tag is repeated verbatim in many places, increasing maintenance overhead and risk of inconsistent updates in the future.
- Suggestion: Consider centralizing the image tag into a single variable or config file referenced by these pipeline definitions to simplify future upgrades and avoid drift.
Best Practices
-
PR description clarity
- File: PR metadata (description)
- Issue: The PR description is minimal and does not clarify the impact or reasoning behind the revert. This can lead to confusion when auditing history.
- Suggestion: Follow best practices by describing why the revert is happening, what issues prompted it, and the plan for reapplying the upgrade if any.
-
Testing coverage and verification
- Files: Not explicitly shown in the diff, but implied pipeline files
- Issue: No mention if the reverted image has been tested to ensure it resolves the issues that prompted this revert. Without verification, this revert might reintroduce old bugs or fail to fix current problems.
- Suggestion: Ensure the revert is accompanied by successful CI runs or test results demonstrating that the Go 1.23 image works correctly, and document that in the PR or commit.
No critical bugs or security issues are evident since this is a rollback of version tags in CI image references. The main improvements revolve around documentation and maintainability.
Replay Test Jobpingcap/tidb
|
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 reverts the previously upgraded TiDB CI images from Go 1.23 to Go 1.25 by changing the container image tags in various Kubernetes pod specs back to use the Go 1.25 images. The changes are straightforward, involving only image tag updates across multiple YAML and Groovy pipeline files. The PR is small, mechanical, and low risk, but it lacks any verification steps or notes on validation of the new images.
Code Improvements
-
Lack of Verification or Testing Notes (Multiple files)
- Files: All
.yamland.groovyfiles where images are updated, e.g.,jenkins/pipelines/ci/tidb/tidb_ghpr_coverage.groovy(line 11),pipelines/pingcap-inc/enterprise-extensions/latest/pod-pr-verify.yaml(line 6), etc. - Issue: The PR simply updates image tags without any indication of testing or validation of the new images. This is important to ensure compatibility and prevent CI breakage.
- Suggestion: Add comments or PR description notes about how the new Go 1.25 images were tested or validated in the CI environment, or reference related test results. Consider adding smoke tests or pipeline job runs results in the PR description to increase confidence.
- Files: All
-
Duplication Across Multiple Files
- Files: Many YAML pod specs and the Groovy pipeline script.
- Issue: The same image tag update is repeated across many files, which can be error-prone and hard to maintain.
- Suggestion: If not already implemented, consider centralizing the image version in a single config file or environment variable that all pipelines reference. This would simplify future upgrades and reduce duplication.
Best Practices
-
PR Description Clarity
- File: PR description
- Issue: The description just states "revert #3989" without context.
- Suggestion: Provide a brief reason why this revert is necessary (e.g., stability issues with Go 1.25 images, bugs found, etc.) to help reviewers and future maintainers understand the motivation.
-
Consistent Commenting in YAML files
- Files: Several pod YAML files contain TODO comments (e.g.,
pod-ghpr_unit_test.yamlline 8). - Issue: The TODOs reference using a standard bazel image for size reduction but remain unaddressed.
- Suggestion: Consider opening a follow-up issue or PR to address these TODOs or add a comment about planned timelines to improve clarity.
- Files: Several pod YAML files contain TODO comments (e.g.,
No critical issues found since this is a low-impact image tag update, but adding validation info and improving maintainability would enhance the quality of this change.
2 participants
revert #3989