feat: use download_pingcap_oci_artifact.sh download artifacts#4145
feat: use download_pingcap_oci_artifact.sh download artifacts#4145lybcodes wants to merge 2 commits intoPingCAP-QE:mainfrom
Conversation
Signed-off-by: lyb <yebin.li@pingcap.com>
Signed-off-by: lyb <yebin.li@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 PR replaces the existing artifact download scripts in several Jenkins pipeline Groovy files with a new centralized script download_pingcap_oci_artifact.sh to fetch PingCAP OCI artifacts. The approach refactors the download steps to use OCI tags computed from PR metadata and standardizes the artifact retrieval mechanism. Overall, the changes improve consistency and maintainability but introduce some repetitive code across pipelines and lack error handling around the new script execution.
Critical Issues
-
Lack of error handling for artifact downloads
- Files:
pull_br_integration_test.groovy(lines ~81),pull_integration_e2e_test.groovy(lines ~40),pull_lightning_integration_test.groovy(lines ~44) - Issue: The shell commands invoking
download_pingcap_oci_artifact.shdo not check for failure explicitly. If downloads fail, subsequent commands (mv, execution of binaries) might cause confusing errors or false positives in the pipeline run. - Suggestion: Add
set -eor explicit error checks after the download command to fail fast on errors. For example:or check exit status explicitly and fail with a descriptive message.set -e ${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --pd=${OCI_TAG_PD} ...
- Files:
-
Silent failure in
mvcommands due to|| true- Files: multiple pipeline scripts, e.g.
pull_br_integration_test.groovy - Issue: Using
mv ... || truesuppresses errors that might arise if the expected files are missing. This could hide problems where artifacts are not downloaded properly. - Suggestion: Remove
|| trueand ensure the script fails if expected binaries are missing, or add checks before moving files to confirm their presence.
- Files: multiple pipeline scripts, e.g.
Code Improvements
-
DRY: Repeated OCI tag computation and environment declarations
- Files: all modified pipeline Groovy files (
pull_br_integration_test.groovy,pull_integration_e2e_test.groovy,pull_lightning_integration_test.groovy) - Issue: The same OCI tag computations and environment variable declarations are repeated in each pipeline script with very similar code.
- Suggestion: Extract common logic into a shared Groovy function or Jenkins shared library step to compute OCI tags and define environment variables once, improving maintainability and reducing duplication.
- Files: all modified pipeline Groovy files (
-
Inconsistent directory handling after downloads
- Issue: The logic to move binaries varies slightly between pipelines (some use
bin, othersthird_bin), and the handling of thetiflashdirectory differs. - Suggestion: Standardize the target binary directory across pipelines or parameterize the directory name to avoid confusion and simplify maintenance.
- Issue: The logic to move binaries varies slightly between pipelines (some use
-
Missing comments/documentation on new script usage
- Issue: The new
download_pingcap_oci_artifact.shscript usage is not explained anywhere in the pipeline scripts. This makes it harder for new contributors to understand the artifact download mechanism. - Suggestion: Add comments describing what the script does, the meaning of OCI tags, and expected directory structures after download.
- Issue: The new
Best Practices
-
Add testing or validation for the new artifact download flow
- Issue: No indication of tests or validation steps added to ensure the new artifact download script works as expected in CI pipelines.
- Suggestion: Add unit or integration tests for
download_pingcap_oci_artifact.shif possible, or pipeline-level smoke tests to verify artifacts are correctly downloaded and executable.
-
Use consistent naming for environment variables
- Issue:
OCI_ARTIFACT_HOSTis declared but never used within the Groovy script snippets. This might cause confusion or misconfiguration. - Suggestion: Either remove unused environment variables or ensure they are referenced properly in the download script or elsewhere.
- Issue:
-
Style: Use multiline Groovy strings consistently
- Issue: Some shell scripts are passed as multiline strings with triple double-quotes (
"""), while others are not documented or inconsistent. - Suggestion: Standardize on multiline strings and format shell commands for readability, including indentations aligned with Groovy's style.
- Issue: Some shell scripts are passed as multiline strings with triple double-quotes (
Summary Action Items:
- Add error handling around
download_pingcap_oci_artifact.shcalls. - Remove or rethink
|| trueonmvcommands to catch missing files. - Extract OCI tag computation and environment setup into reusable code.
- Add comments explaining the artifact download process.
- Validate the new download script usage with tests or pipeline checks.
- Clean up unused environment variables.
- Standardize directory and naming conventions across pipelines.
Summary of ChangesHello @lybcodes, 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 modernizes the artifact retrieval process across several critical Jenkins integration test pipelines. By migrating from a legacy file server to an OCI artifact registry and implementing a new, unified download script, it standardizes how components like PD, TiKV, TiFlash, and TiCDC are fetched. This change enhances the reliability and maintainability of the build system, ensuring that tests consistently use correctly versioned artifacts and streamlining future updates to the artifact distribution method. Highlights
Changelog
Activity
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 refactors the CI pipelines to download dependencies as OCI artifacts instead of from a file server. This is a great improvement for reliability and standardization of CI artifacts. My review includes suggestions to improve the robustness of the shell scripts used for handling these artifacts by removing unnecessary error suppression, which will make potential issues easier to debug.
| sh label: "download third_party", script: """ | ||
| ${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --pd=${OCI_TAG_PD} --tikv=${OCI_TAG_TIKV} --tiflash=${OCI_TAG_TIFLASH} | ||
| rm -rf bin/ && mkdir -p bin | ||
| mv pd-server tikv-server bin/ 2>/dev/null || true |
There was a problem hiding this comment.
The error suppression with 2>/dev/null || true can hide real issues. The download_pingcap_oci_artifact.sh script uses set -eo pipefail and should exit with an error if it fails to download and extract the required binaries. Therefore, pd-server and tikv-server are expected to exist. If they don't, it indicates a problem that should cause the build to fail immediately. Please remove the error suppression to make the script more robust.
mv pd-server tikv-server bin/
| sh label: 'download binary', script: """ | ||
| ${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --pd=${OCI_TAG_PD} --tikv=${OCI_TAG_TIKV} --tiflash=${OCI_TAG_TIFLASH} --ticdc=${OCI_TAG_TICDC} | ||
| mkdir -p third_bin | ||
| mv pd-server tikv-server cdc third_bin/ 2>/dev/null || true |
There was a problem hiding this comment.
The error suppression with 2>/dev/null || true can hide real issues. The download_pingcap_oci_artifact.sh script uses set -eo pipefail and should exit with an error if it fails to download and extract the required binaries. Therefore, pd-server, tikv-server, and cdc are expected to exist. If they don't, it indicates a problem that should cause the build to fail immediately. Please remove the error suppression to make the script more robust.
mv pd-server tikv-server cdc third_bin/
| sh label: "download third_party", script: """ | ||
| ${WORKSPACE}/scripts/artifacts/download_pingcap_oci_artifact.sh --pd=${OCI_TAG_PD} --tikv=${OCI_TAG_TIKV} --tiflash=${OCI_TAG_TIFLASH} | ||
| mkdir -p bin | ||
| mv pd-server tikv-server bin/ 2>/dev/null || true |
There was a problem hiding this comment.
The error suppression with 2>/dev/null || true can hide real issues. The download_pingcap_oci_artifact.sh script uses set -eo pipefail and should exit with an error if it fails to download and extract the required binaries. Therefore, pd-server and tikv-server are expected to exist. If they don't, it indicates a problem that should cause the build to fail immediately. Please remove the error suppression to make the script more robust.
mv pd-server tikv-server bin/
1 participant
No description provided.