ci: add replay-jenkins-build script and presubmit job#4194
ci: add replay-jenkins-build script and presubmit job#4194ti-chi-bot[bot] merged 2 commits intomainfrom
Conversation
Add .ci/replay-jenkins-build.sh to replay historical Jenkins pipeline builds. Supports single-script replay and --auto-changed mode to replay all changed pipelines from a git diff, handles auth/crumb, queue waiting, and optional --wait for final build result. Update docs (docs/contributing.md, docs/guides/CI.md) with recommended pre-PR checks and examples for static validation and real replay testing. Add optional presubmit pull-replay-jenkins-pipelines for PingCAP-QE/ci that runs against jenkins-beta, is triggerable via PR comment, and limits replays to a configurable max (default 20).
![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 introduces a script (replay-jenkins-build.sh) for replaying Jenkins pipeline builds, enhances documentation for pre-PR checks, and updates CI configurations with new Prow jobs for validating pipeline changes. The approach is well-structured, focusing on automation, maintainability improvements, and detailed instructions for contributors. Overall, the changes are functional and comprehensive but could benefit from minor improvements in error handling and clarity in certain areas.
Critical Issues
-
File:
.ci/replay-jenkins-build.sh, Line: 391
Issue: Thesubmit_replayfunction does not validate the success or failure of the replay submission. If the Jenkins API fails or returns an unexpected response, the script may proceed without detecting the issue.
Why: This can lead to silent failures, especially if Jenkins rejects the replay request.
Suggested Fix: Add explicit error handling to check the status code and response content of theapi_post_script_textcall:local response response="$(api_post_script_text "$groovy_file")" if [[ -z "$response" ]]; then fatal "Failed to submit replay. No response from Jenkins API." fi
-
File:
.ci/replay-jenkins-build.sh, Line: 479
Issue: Thewait_queue_to_build_urlmethod does not handle edge cases where the Jenkins queue item endlessly stays in the queue due to configuration or resource constraints.
Why: This could result in the script hanging indefinitely, especially if the job cannot be executed due to Jenkins resource limits or misconfiguration.
Suggested Fix: Add a timeout mechanism with clearer error messages:if (( now - started > timeout_sec )); then fatal "Timeout waiting for queue item to start. Ensure Jenkins has sufficient resources or validate job configurations." fi
Code Improvements
-
File:
.ci/replay-jenkins-build.sh, Line: 101
Issue: The functionscript_to_job_pathassumes a strict directory structure for pipeline files without fallback handling or validation for unexpected paths.
Why: This could break if the directory structure is modified or if contributors use non-standard paths.
Suggested Fix: Add fallback validation and a helpful error message:if [[ "$rel" != pipelines/* ]]; then fatal "Unexpected script path structure: ${script_file}. Verify pipeline file locations." fi
-
File:
.ci/replay-jenkins-build.sh, Line: 599
Issue: The script usesjqfor parsing JSON responses but lacks fallback handling ifjqfails to parse or is unavailable.
Why: On systems wherejqis misconfigured or unavailable, the script would fail unexpectedly.
Suggested Fix: Add a check forjqfunctionality and fallback to manual parsing if necessary:require_bin jq || fatal "jq is required for JSON parsing. Please install it before running the script."
Best Practices
-
File:
.ci/replay-jenkins-build.sh, Line: 15
Issue: The script lacks comments explaining key functions likediscover_changed_scriptsandsubmit_replay.
Why: These functions have complex logic that could confuse new contributors.
Suggested Fix: Add brief comments above each function:# Discover changed pipeline scripts between two Git SHAs discover_changed_scripts() { ... }
-
File:
docs/contributing.md, Line: 10
Issue: The documentation suggests usingJENKINS_USERandJENKINS_TOKENbut does not explain how to obtain these credentials securely.
Why: This could lead to misuse or security risks if credentials are shared improperly.
Suggested Fix: Include a note on obtaining and securely storing credentials:Note: Obtain `JENKINS_USER` and `JENKINS_TOKEN` from your Jenkins account and store them securely using environment variables or secret management tools.
-
File:
prow-jobs/pingcap-qe/ci/presubmits.yaml, Line: 8
Issue: Thepull-replay-jenkins-pipelinesjob does not document its purpose clearly in the YAML file.
Why: Future maintainers might struggle to understand its intent without referring to external documentation.
Suggested Fix: Add a descriptive comment:# This presubmit job replays Jenkins pipeline builds for changed pipeline files in a PR.
Additional Notes
Overall, the PR is impactful and well-organized. Addressing the above issues will further improve robustness, maintainability, and clarity for contributors and maintainers.
Summary of ChangesHello, 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 enhances the CI/CD workflow by introducing a dedicated script and an optional Prow presubmit job for automated replay testing of Jenkins pipeline changes. It also provides updated documentation to guide developers through the process of validating their pipeline modifications locally and within the CI system, aiming to improve the reliability and consistency of pipeline development and deployment. Highlights
Changelog
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 introduces a valuable replay-jenkins-build.sh script and an associated Prow job to facilitate testing of Jenkins pipeline changes. The accompanying documentation updates are clear and helpful. The overall changes improve the CI/CD workflow for pipeline development. I've identified a few areas for improvement in the new script to enhance its maintainability and a violation of CI best practices regarding dependency management. My detailed comments are below.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.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 introduces a replay-jenkins-build.sh script to facilitate automated replay testing for Jenkins pipeline changes. It also updates documentation for contributors and adds a new optional presubmit job (pull-replay-jenkins-pipelines) to Prow for improved CI validation. The implementation delivers a robust mechanism for replay validation, clear documentation, and enhanced presubmit job configuration for maintainability. Code quality is generally strong, though there are areas where edge cases and error handling can be improved.
Critical Issues
-
Error Handling in Replay Submission (
.ci/replay-jenkins-build.sh, line 620):- Problem: If the
submit_replay()function fails, the script exits without providing actionable feedback for debugging. - Why it’s an issue: Users will face difficulty identifying the root cause of a failure, especially when interacting with Jenkins APIs.
- Suggested Fix:
Add detailed error logs tofatal()calls withinsubmit_replay()related sections:fatal "Replay submission failed: Unable to submit replay for build ${build_url}. Check script file and authentication."
- Problem: If the
-
Authentication Requirement (
.ci/replay-jenkins-build.sh, line 467):- Problem: The script requires both
JENKINS_USERandJENKINS_TOKENfor submission but does not validate their presence early in all relevant paths. - Why it’s an issue: Missing validation could lead to runtime failures in
submit_replay()or other dependent functions. - Suggested Fix:
ValidateJENKINS_USERandJENKINS_TOKENexplicitly invalidate_inputs():if [[ "$AUTO_CHANGED" != "true" && "$DRY_RUN" != "true" ]]; then [[ -n "$JENKINS_USER" && -n "$JENKINS_TOKEN" ]] || fatal "JENKINS_USER and JENKINS_TOKEN are required for replay submit." fi
- Problem: The script requires both
Code Improvements
-
Edge Case for Empty
JENKINS_URL(.ci/replay-jenkins-build.sh, line 607):- Problem: The default
JENKINS_URLis set to a hardcoded value, but an empty environment variable could overwrite it without warning. - Suggested Fix: Add a fallback mechanism to ensure
JENKINS_URLis never empty:JENKINS_URL="${JENKINS_URL:-https://do.pingcap.net/jenkins}" [[ -n "$JENKINS_URL" ]] || fatal "JENKINS_URL cannot be empty."
- Problem: The default
-
Performance Optimization for Changed Script Discovery (
.ci/replay-jenkins-build.sh, line 171):- Problem: Using
git diff | rgto discover script changes could be inefficient for large repos. - Suggested Fix: Use
git diff --diff-filter=dto exclude deleted files upfront:git diff --diff-filter=d --name-only "$base_sha" "$head_sha" | rg '^pipelines/.*\.groovy$' || true
- Problem: Using
-
Refactor Excessive Nested Conditionals (
.ci/replay-jenkins-build.sh, lines 419–620):- Problem: The
replay_one()function contains deeply nested conditionals, making it harder to read and maintain. - Suggested Fix: Break down the function into smaller helper functions, e.g.,
get_build_url()andvalidate_script_file().
- Problem: The
Best Practices
-
Documentation for Environment Variables (
docs/contributing.md, line 13):- Problem: The description of
JENKINS_USERandJENKINS_TOKENin the replay instructions is insufficient for users unfamiliar with Jenkins API. - Suggested Fix: Expand the documentation to clarify how to generate a Jenkins API token and where to configure it.
- Problem: The description of
-
Testing Coverage for Edge Cases (
prow-jobs/pingcap-qe/ci/presubmits.yaml, line 12):- Problem: The new presubmit job lacks explicit error handling for scenarios like missing credentials or non-existent pipeline files.
- Suggested Fix: Add a test case to ensure the job gracefully exits for these edge cases, logging actionable errors.
-
Naming Convention in YAML Anchors (
prow-jobs/pingcap-qe/ci/presubmits.yaml, line 1):- Problem: The anchor name
labelsis generic and may lead to confusion if reused in unrelated contexts. - Suggested Fix: Use a more descriptive name:
labels: &jenkins-qe-labels
- Problem: The anchor name
-
Command Validation (
.ci/replay-jenkins-build.sh, line 91):- Problem: The
require_bin()function doesn’t provide fallback instructions if a required binary is missing. - Suggested Fix: Extend error messages to include installation instructions:
fatal "missing required command: ${bin}. Please install ${bin} using your package manager."
- Problem: The
These changes will improve robustness, maintainability, and user experience while ensuring the functionality remains secure and performant.
|
/approve |
|
[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 |
1 participant
This pull request improves the documentation and CI workflow for Jenkins pipeline changes, and refactors the Prow job configuration to support better pre-PR validation and replay testing. The main focus is to ensure that changes to
pipelines/**/*.groovyfiles are thoroughly validated both locally and in presubmit jobs, and to clarify the process for contributors. Additionally, the Prow job YAML files are refactored for maintainability.Documentation enhancements:
docs/contributing.mdrecommending local static and replay checks for Jenkins pipeline changes, with command examples and links to further documentation.docs/guides/CI.mdwith detailed instructions on pre-PR verification, including static validation, replay testing, and how to use Prow jobs for Jenkins pipeline changes.CI and Prow job improvements:
pull-replay-jenkins-pipelinesinprow-jobs/pingcap-qe/ci/presubmits.yamlto automate replay validation of changed pipeline files, including job configuration, environment variables, and usage of secrets.labelsanchor for consistency and maintainability, updating all job definitions accordingly.prow-jobs/tikv/pd/common-presubmits.yamlto clarify section separation and maintain consistency.