-
Notifications
You must be signed in to change notification settings - Fork 110
refactor: encapsulate cloudevents sending into shared function #4134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
- Add sendTestCaseRunReport() function to prow.groovy library - Replace inline curl commands with function call in all pipeline files - CloudEvents URL centralized: https://internal2-do.pingcap.net/cloudevents-server/events - Benefits: * Reduced code duplication (~85% reduction from 12 lines to 2 lines) * Single source of truth for CloudEvents URL * Easier maintenance and updates
|
[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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have already done a preliminary review for you, and I hope to help you do a better job.
Summary
This pull request refactors the Jenkins pipeline scripts to encapsulate the task of sending CloudEvents into a reusable shared function, sendTestCaseRunReport, defined within prow.groovy. It replaces verbose inline curl commands across ~23 pipeline files with concise function calls, centralizes CloudEvents URL configuration, and reduces code duplication significantly. The overall quality of these changes is good, with improvements in readability, maintainability, and consistency. However, there are minor areas that could be improved for better robustness and adherence to best practices.
Critical Issues
No critical issues were identified in the proposed changes. The refactoring appears to work as intended and does not introduce bugs, security vulnerabilities, or performance bottlenecks.
Code Improvements
1. Error Handling in sendTestCaseRunReport
- File:
libraries/tipipeline/vars/prow.groovy(lines 260-272) - Issue: The
curlcommand in the new shared function uses|| trueto suppress all errors, which could mask legitimate issues during CloudEvents reporting. This makes debugging difficult if the server is unavailable or if there is a malformed request. - Suggested Solution: Implement better error handling to log failures explicitly or retry the operation. For example, capture the exit code and provide meaningful logs:
def sendTestCaseRunReport(repo, branch, dataFile = 'bazel-go-test-problem-cases.json') { try { sh label: 'Send event to cloudevents server', script: """timeout 10 \ curl --verbose --request POST --url https://internal2-do.pingcap.net/cloudevents-server/events \ --header "ce-id: \$(uuidgen)" \ --header "ce-source: \${JENKINS_URL}" \ --header 'ce-type: test-case-run-report' \ --header 'ce-repo: ${repo}' \ --header 'ce-branch: ${branch}' \ --header "ce-buildurl: \${BUILD_URL}" \ --header 'ce-specversion: 1.0' \ --header 'content-type: application/json; charset=UTF-8' \ --data @${dataFile} """ } catch (Exception e) { echo "Error sending CloudEvents report: ${e.message}" throw e } }
2. Parameter Validation
- File:
libraries/tipipeline/vars/prow.groovy(lines 260-272) - Issue: The function does not validate the
repo,branch, ordataFileparameters before constructing thecurlcommand. Invalid or empty parameters could lead to runtime failures or incorrect API requests. - Suggested Solution: Add validation for input parameters:
if (!repo || !branch || !dataFile) { error("Invalid arguments passed to sendTestCaseRunReport: repo=${repo}, branch=${branch}, dataFile=${dataFile}") }
Best Practices
1. Improve Documentation
- File:
libraries/tipipeline/vars/prow.groovy(lines 260-272) - Issue: The newly added function lacks detailed comments or documentation about its purpose, parameters, and expected behavior.
- Suggested Solution: Add a clear comment block above the function to explain its usage:
/** * Sends a test case run report to the CloudEvents server. * * @param repo Repository identifier (e.g., "pingcap/tidb"). * @param branch Branch name (e.g., "master"). * @param dataFile Path to the JSON file containing report data (default: 'bazel-go-test-problem-cases.json'). */ def sendTestCaseRunReport(repo, branch, dataFile = 'bazel-go-test-problem-cases.json') { ... }
2. Testing Coverage
- Issue: The new function is used in multiple pipelines, but there is no indication of added tests to ensure the function behaves correctly under different scenarios (e.g., invalid inputs, server unavailability). This could lead to undetected errors in production pipelines.
- Suggested Solution: Add unit tests for the
sendTestCaseRunReportfunction within the shared library testing framework. Mock theshcommand to simulate various scenarios, including success and failure.
3. Centralize Timeout Configuration
- File:
libraries/tipipeline/vars/prow.groovy(line 261) - Issue: The timeout value (
timeout 10) is hardcoded, making it difficult to adjust globally. If the timeout needs to be updated in the future, each usage of the function must be revisited. - Suggested Solution: Use a configurable parameter or global constant for the timeout value:
def timeoutSeconds = 10 sh label: 'Send event to cloudevents server', script: """timeout ${timeoutSeconds} ...
4. Naming Conventions
- File:
libraries/tipipeline/vars/prow.groovy(line 260) - Issue: The function name
sendTestCaseRunReportis descriptive but could be more precise about its dependency on CloudEvents. - Suggested Solution: Consider renaming the function to
sendCloudEventTestReportfor clarity.
Conclusion
The refactoring significantly improves pipeline maintainability and reduces code duplication. However, error handling, documentation, and testing should be enhanced to ensure robustness, clarity, and reliability. Addressing these issues will make the shared function more resilient and easier to use across the pipelines.
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 improves the maintainability and readability of the CI/CD pipelines by refactoring the mechanism for sending test case run reports to a CloudEvents server. By encapsulating the 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
|
|
@copilot fix the PR description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a great refactoring effort. It successfully encapsulates the logic for sending CloudEvents into a shared function sendTestCaseRunReport in prow.groovy. This significantly reduces code duplication across numerous pipeline files and centralizes the endpoint URL, which improves maintainability. The changes are well-executed and make the pipeline code cleaner and more concise. I have one suggestion to make the new shared function more robust and the CI logs cleaner.
| // send test case run report to cloudevents server | ||
| def sendTestCaseRunReport(repo, branch, dataFile = 'bazel-go-test-problem-cases.json') { | ||
| sh label: 'Send event to cloudevents server', script: """timeout 10 \ | ||
| curl --verbose --request POST --url https://internal2-do.pingcap.net/cloudevents-server/events \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using --verbose for curl can make CI logs unnecessarily noisy. It's generally better to use -sS (--silent --show-error) to suppress progress output but still show errors. Additionally, adding the -f (--fail) flag will ensure that curl returns a non-zero exit code for HTTP server errors (like 4xx or 5xx), which is a more robust way to detect failures, even with || true.
curl -sSf --request POST --url https://internal2-do.pingcap.net/cloudevents-server/events \
Summary
sendTestCaseRunReport()function toprow.groovyshared libraryhttps://internal2-do.pingcap.net/cloudevents-server/eventsChanges
Benefits
Modified Files
libraries/tipipeline/vars/prow.groovy- Added shared functionExample
Before:
After:
script { prow.sendTestCaseRunReport("${REFS.org}/${REFS.repo}", "${REFS.base_ref}") }