refactor: use download_pingcap_oci_artifact.sh instead of download_pingcap_artifact.sh#4136
refactor: use download_pingcap_oci_artifact.sh instead of download_pingcap_artifact.sh#4136
Conversation
This adds the utils container (ghcr.io/pingcap-qe/cd/utils/release:v2025.10.12-7-gfdd779c) to all pod templates used by integration tests that will use the new download_pingcap_oci_artifact.sh script. Affected branches: - pingcap/tidb: release-6.5, 8.2, 8.3, 8.4, 9.0-beta - tikv/pd: latest, release-7.1 - pingcap-qe/tidb-test: release-6.0, 6.2
- Replace download_pingcap_artifact.sh with download_pingcap_oci_artifact.sh
- Use container("utils") to execute the download script in dir("bin")
- Add OCI_ARTIFACT_HOST environment variable
- Remove FILE_SERVER_URL environment variable
- Remove chmod step (script permissions are set in git)
- Update version check commands to use relative paths (./tikv-server -V)
Affected branches:
- pingcap/tidb: release-6.5, 8.2, 8.3, 8.4, 9.0-beta
- tikv/pd: latest, release-7.1
- pingcap-qe/tidb-test: release-6.0, 6.2
|
I Skip it since the diff size(157962 bytes > 80000 bytes) is too large |
for more information, see https://pre-commit.ci
|
I Skip it since the diff size(157850 bytes > 80000 bytes) is too large |
|
[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 |
1 similar comment
|
[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 |
Summary of ChangesHello @wuhuizuo, 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 significantly refactors the Jenkins pipeline configurations by migrating from an older artifact download script to a new OCI-based one. The changes streamline the artifact retrieval process, standardize the environment across various integration tests, and improve the maintainability of the CI/CD pipelines by centralizing utility functions within a dedicated container. 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
|
|
@wuhuizuo: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
![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 refactors numerous Jenkins pipeline jobs to use a new artifact download script. While the intent is good, the execution has introduced several critical errors across many pipeline files. The most common issues are syntactically incorrect script blocks in Groovy files, incorrect paths to binaries, incomplete logic for moving downloaded artifacts, and incorrect restructuring of pipeline stages which moves steps out of their necessary container environments. These errors appear to be the result of a systematic but flawed automated change and will likely cause many CI jobs to fail. I've highlighted several examples of these patterns in my comments, but a thorough review and correction of all changed files is required.
| dir("bin") { container("utils") { sh label: 'download binary', script: """ | ||
| container("utils") { | ||
| sh label: 'download binary', script: "${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --pd=${REFS.base_ref} --tikv=${REFS.base_ref}" | ||
|
|
||
|
|
||
|
|
||
| ./tikv-server -V | ||
| ./pd-server -V | ||
| """ | ||
| } | ||
| } |
There was a problem hiding this comment.
This script block is syntactically incorrect. It appears to have a nested Groovy container and sh step within the shell script string, which will cause the pipeline to fail. The entire block needs to be refactored to have a valid structure, with only shell commands inside the sh script's triple quotes.
| sh label: 'download binary', script: """ | ||
| chmod +x ${WORKSPACE}/scripts/artifacts/*.sh | ||
| ${WORKSPACE}/scripts/artifacts/download_pingcap_artifact.sh --pd=${REFS.base_ref} --tikv=${REFS.base_ref} | ||
| container("utils") { | ||
| sh label: 'download binary', script: "${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --pd=${REFS.base_ref} --tikv=${REFS.base_ref}" | ||
| rm -rf bin/ && mkdir -p bin/ | ||
| mv third_bin/tikv-server bin/ | ||
| mv third_bin/pd-server bin/ | ||
| mkdir -p bin | ||
|
|
||
| mv pd-server bin/ | ||
| ls -alh bin/ | ||
| chmod +x bin/* | ||
| ./bin/tikv-server -V | ||
| ./bin/pd-server -V | ||
| ./tikv-server -V | ||
| ./pd-server -V | ||
| """ |
There was a problem hiding this comment.
This script block is syntactically incorrect due to a nested container and sh step within the script string. Additionally, the logic is flawed: it attempts to move pd-server but not tikv-server, and then tries to check the version of tikv-server from a location it hasn't been moved to. This will cause multiple failures.
| sh label: 'download tikv', script: """ | ||
| ${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --tikv=${REFS.base_ref} | ||
| mkdir -p bin | ||
|
|
||
| mv pd-server bin/ | ||
| ls -alh bin/ | ||
| bin/pd-server -V | ||
| bin/tikv-server -V | ||
| """ |
There was a problem hiding this comment.
The logic in this script is incorrect. It attempts to move pd-server, which is not downloaded in this step. It also downloads tikv-server but fails to move it into the bin directory. This will cause the version checks and subsequent test stages to fail.
sh label: 'download tikv', script: """
${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --tikv=${REFS.base_ref}
mv tikv-server bin/
ls -alh bin/
bin/pd-server -V
bin/tikv-server -V
"""
| dir("bin") { container("utils") { sh label: 'download binary', script: """ | ||
| container("utils") { | ||
| sh label: 'download binary', script: "${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --pd=${REFS.base_ref} --tikv=${REFS.base_ref}" | ||
|
|
||
|
|
||
|
|
||
| ./tikv-server -V | ||
| ./pd-server -V | ||
| """ | ||
| } |
There was a problem hiding this comment.
This script block is syntactically incorrect. It appears to have a nested Groovy container and sh step within the shell script string, which will cause the pipeline to fail. The entire block needs to be refactored to have a valid structure, with only shell commands inside the sh script's triple quotes.
| sh label: 'download binary', script: """ | ||
| chmod +x ${WORKSPACE}/scripts/artifacts/*.sh | ||
| ${WORKSPACE}/scripts/artifacts/download_pingcap_artifact.sh --pd=${REFS.base_ref} --tikv=${REFS.base_ref} | ||
| container("utils") { | ||
| sh label: 'download binary', script: "${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --pd=${REFS.base_ref} --tikv=${REFS.base_ref}" | ||
| rm -rf bin/ && mkdir -p bin/ | ||
| mv third_bin/tikv-server bin/ | ||
| mv third_bin/pd-server bin/ | ||
| mkdir -p bin | ||
|
|
||
| mv pd-server bin/ | ||
| ls -alh bin/ | ||
| chmod +x bin/* | ||
| ./bin/tikv-server -V | ||
| ./bin/pd-server -V | ||
| ./tikv-server -V | ||
| ./pd-server -V | ||
| """ |
There was a problem hiding this comment.
This script block is syntactically incorrect due to a nested container and sh step within the script string. Additionally, the logic is flawed: it attempts to move pd-server but not tikv-server, and then tries to check the version of tikv-server from a location it hasn't been moved to. This will cause multiple failures.
| sh label: 'download binary', script: """ | ||
| chmod +x ${WORKSPACE}/scripts/artifacts/*.sh | ||
| ${WORKSPACE}/scripts/artifacts/download_pingcap_artifact.sh --pd=${REFS.base_ref} --tikv=${REFS.base_ref} | ||
| mv third_bin/tikv-server bin/ | ||
| mv third_bin/pd-server bin/ | ||
| container("utils") { | ||
| sh label: 'download binary', script: "${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --pd=${REFS.base_ref} --tikv=${REFS.base_ref}" | ||
|
|
||
|
|
||
|
|
||
| ls -alh bin/ | ||
| chmod +x bin/* | ||
| ./bin/tidb-server -V | ||
| ./bin/tikv-server -V | ||
| ./bin/pd-server -V | ||
| ./tikv-server -V | ||
| ./pd-server -V | ||
| """ |
There was a problem hiding this comment.
This script block is broken. It has nested Groovy syntax inside a shell script string, which is invalid. It also fails to move the downloaded binaries, so the version checks for tikv-server and pd-server will fail as they won't be in the expected path.
| sh label: 'download binary', script: """ | ||
| chmod +x ${WORKSPACE}/scripts/artifacts/*.sh | ||
| ${WORKSPACE}/scripts/artifacts/download_pingcap_artifact.sh --pd=${REFS.base_ref} --tikv=${REFS.base_ref} | ||
| mv third_bin/tikv-server bin/ | ||
| mv third_bin/pd-server bin/ | ||
| container("utils") { | ||
| sh label: 'download binary', script: "${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --pd=${REFS.base_ref} --tikv=${REFS.base_ref}" | ||
| mkdir -p bin | ||
|
|
||
| mv pd-server bin/ | ||
| ls -alh bin/ | ||
| chmod +x bin/* | ||
| """ |
There was a problem hiding this comment.
This script block is syntactically incorrect with nested Groovy syntax. It also only moves pd-server and not tikv-server, which will cause failures in later steps that require tikv-server.
| container('nodejs') { | ||
| dir('tidb') { | ||
| sh label: 'tidb-server', script: '[ -f bin/tidb-server ] || make' | ||
| retry(2) { | ||
| sh label: 'download binary', script: """ | ||
| chmod +x ${WORKSPACE}/scripts/artifacts/*.sh | ||
| ${WORKSPACE}/scripts/artifacts/download_pingcap_artifact.sh --pd=${REFS.base_ref} --tikv=${REFS.base_ref} | ||
| mv third_bin/tikv-server bin/ | ||
| mv third_bin/pd-server bin/ | ||
| rm -rf bin/bin | ||
| ls -alh bin/ | ||
| chmod +x bin/* | ||
| """ | ||
| } | ||
| } | ||
| dir('tidb-test') { | ||
| cache(path: "./", includes: '**/*', key: "ws/${BUILD_TAG}/tidb-test") { | ||
| sh label: "prepare", script: """ | ||
| touch ws-${BUILD_TAG} | ||
| mkdir -p bin | ||
| cp ${WORKSPACE}/tidb/bin/* bin/ && chmod +x bin/* | ||
| ls -alh bin/ | ||
| ./bin/tidb-server -V | ||
| ./bin/tikv-server -V | ||
| ./bin/pd-server -V | ||
| """ | ||
| } | ||
| dir('tidb') { | ||
| dir("bin") { | ||
| container('utils') { | ||
| retry(2) { | ||
| sh label: 'download binary', script: """ | ||
| ${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --pd=${REFS.base_ref} --tikv=${REFS.base_ref} | ||
| ls -alh | ||
| chmod +x * | ||
| """ | ||
| } | ||
| } | ||
| } | ||
| } | ||
| dir('tidb-test') { | ||
| cache(path: "./", includes: '**/*', key: "ws/${BUILD_TAG}/tidb-test") { | ||
| sh label: "prepare", script: """ | ||
| touch ws-${BUILD_TAG} | ||
| mkdir -p bin | ||
| cp ${WORKSPACE}/tidb/bin/* bin/ && chmod +x bin/* | ||
| ls -alh bin/ | ||
| ./bin/tidb-server -V | ||
| ./tikv-server -V | ||
| ./pd-server -V | ||
| """ | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The refactoring has moved the dir('tidb-test') block outside of the container('nodejs'). The tests for Node.js are executed in later stages and will likely fail because they are no longer in the correct container environment. The dir('tidb') block for downloading binaries and the dir('tidb-test') block should be within the container('nodejs') scope.
| sh label: 'download binary', script: """ | ||
| chmod +x ${WORKSPACE}/scripts/artifacts/*.sh | ||
| ${WORKSPACE}/scripts/artifacts/download_pingcap_artifact.sh --pd=${REFS.base_ref} --tikv=${REFS.base_ref} | ||
| mv third_bin/tikv-server bin/ | ||
| mv third_bin/pd-server bin/ | ||
| ${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --pd=${REFS.base_ref} --tikv=${REFS.base_ref} | ||
|
|
||
|
|
||
|
|
||
| rm -rf bin/bin | ||
| ls -alh bin/ | ||
| chmod +x bin/* | ||
| """ |
There was a problem hiding this comment.
This script downloads binaries but does not move them into the bin directory. As a result, the subsequent commands ls -alh bin/ and chmod +x bin/* will likely fail because the bin directory will be empty or non-existent, and later stages will not find the required binaries.
| dir('tidb-test/mysql_test') { | ||
| sh """ | ||
| mkdir -p bin | ||
|
|
There was a problem hiding this comment.
The mkdir -p bin command has been removed. The subsequent mv command will fail if the bin directory doesn't exist. It's safer to restore this command to ensure the directory is created before moving files into it.
mkdir -p bin
1 participant
Changes
This PR refactors all Jenkins pipeline jobs that use to use the new script.
Key Changes
Pod Templates (53 files): Added container to all integration test pod templates
Pipeline Groovy Files (42 files):
Affected Branches
Commits