feat(dnm/review): do-not-merge review label

[timzxz]

No description provided.

[ti-chi-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign dillon-zheng for approval. For more information see the Code Review Process.
Please ensure that each of them provides their approval before proceeding.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[ti-chi-bot]

I have already done a preliminary review for you, and I hope to help you do a better job.

Summary

This PR introduces a new Tekton task and associated configurations to enforce a do-not-merge/request-change label based on CodeRabbit bot reviews. The implementation involves several YAML files defining tasks, triggers, and rollout policies, along with a detailed guide explaining the system's logic. The approach is technically sound and adheres to best practices for Tekton and GitHub API integration. However, there are critical issues around error handling, edge case management, and rollout readiness that need to be addressed for smoother operation and maintainability.

Feedback

Critical Issues

1. Error Handling in Bash Script (tekton/v1/tasks/ci/coderabbit-review-label.yaml, lines 23-121):

   • Issue: The script relies on trap cleanup EXIT to restore the label if an error occurs during execution, but this approach assumes the add_label function will succeed during cleanup. If the API call fails, the PR could remain incorrectly labeled.
   • Suggestion: Implement retries for the add_label function in the cleanup logic to handle transient API errors.

     add_label() {
       local retries=3
       for attempt in $(seq 1 $retries); do
         gh api --method POST "$endpoint/issues/$PR_NUMBER/labels" -f "labels[]=$label" >/dev/null && return
         sleep 2
       done
       echo "Failed to add label after $retries attempts" >&2
       return 1
     }

2. Concurrent TaskRun Handling (tekton/v1/tasks/ci/coderabbit-review-label.yaml, lines 95-121):

   • Issue: The script does not account for concurrent TaskRuns potentially modifying the label state simultaneously, leading to race conditions.
   • Suggestion: Use optimistic locking or introduce a mechanism to serialize TaskRuns for the same PR. For example, a lock file or state stored in an external database could prevent simultaneous modifications.

3. Rollout Repository Filter Logic (tekton/v1/triggers/triggers/env-gcp/_/github-pr-coderabbit-review-label.yaml, lines 10-33):

   • Issue: The repository filter is currently disabled (body.repository.full_name in []). If enabled incorrectly, it could apply the label to unintended repositories.
   • Suggestion: Validate the repository list before enabling the filter and ensure proper testing in pilot repositories.

Code Improvements

1. Validation Logic (tekton/v1/tasks/ci/coderabbit-review-label.yaml, lines 31-37):

   • Issue: Validation of PR_OWNER, PR_REPO, and PR_NUMBER uses regex checks but lacks comprehensive error messages when invalid.
   • Suggestion: Add descriptive error messages to aid debugging.

     [[ "$PR_OWNER" =~ ^[A-Za-z0-9-]+$ ]] || { echo "Invalid PR_OWNER: $PR_OWNER" >&2; exit 1; }
     [[ "$PR_REPO" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "Invalid PR_REPO: $PR_REPO" >&2; exit 1; }
     [[ "$PR_NUMBER" =~ ^[0-9]+$ ]] || { echo "Invalid PR_NUMBER: $PR_NUMBER" >&2; exit 1; }

2. Retry Logic for API Calls (tekton/v1/tasks/ci/coderabbit-review-label.yaml, lines 59-63):

   • Issue: The script does not implement retries for GitHub API calls beyond the snapshot function. This could result in failed TaskRuns due to transient network issues.
   • Suggestion: Add retry logic to all GitHub API calls (snapshot, add_label, and gh api --method DELETE) to improve robustness.

Best Practices

1. Testing Coverage Missing:

   • Issue: There is no mention of how this functionality will be tested before rollout, especially for edge cases like concurrent merges or API failures.
   • Suggestion: Include a testing plan in the documentation and consider adding automated tests for the bash script using tools like bats or unit tests for individual functions.

2. Documentation (docs/guides/coderabbit-review-label.md, lines 1-59):

   • Issue: While the guide is detailed, it does not provide examples of expected input/output or error scenarios.
   • Suggestion: Add a section with example payloads and step-by-step walkthroughs for typical use cases (e.g., PR opened, approval added, changes requested).

3. Hardcoded Label Name (tekton/v1/tasks/ci/coderabbit-review-label.yaml, lines 32 and 48):

   • Issue: The label name do-not-merge/request-change is hardcoded multiple times, making it difficult to change in the future.
   • Suggestion: Use a parameter or environment variable to define the label name.

     params:
       - name: label-name
         type: string

     label="${LABEL_NAME:-do-not-merge/request-change}"

4. Trigger Template Configuration (tekton/v1/triggers/templates/_/coderabbit-review-label.yaml, lines 1-28):

   • Issue: The workspaces.github.secretName is hardcoded as github, which may cause issues in environments with different secret names.
   • Suggestion: Parameterize the secret name to allow flexibility.

     workspaces:
       - name: github
         secret:
           secretName: $(tt.params.github-secret-name)
     params:
       - name: github-secret-name

5. Code Duplication (tekton/v1/tasks/ci/coderabbit-review-label.yaml, lines 79-121):

   • Issue: The snapshot and label reconciliation logic appear repetitive.
   • Suggestion: Break the script into smaller reusable functions to improve readability and maintainability.

Conclusion

The PR introduces a useful feature but requires improvements in error handling, concurrency management, documentation, and testing to ensure robustness and clarity. Addressing these issues will make the system more reliable and easier to maintain. Prioritize fixing the critical issues and validating the rollout process before deployment.

[timzxz]

/hold

[ti-chi-bot]

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 Tekton-based mechanism to manage a do-not-merge/request-change review label in GitHub PRs. The implementation leverages GitHub API integrations and label reconciliation logic to ensure PRs blocked by CodeRabbit's review cannot be merged unless explicitly cleared by an approval tied to the latest commit. The approach is detailed and comprehensive, but the PR description is missing essential context that would aid understanding. The code quality is solid overall, but there are areas for improvement in error handling, edge case management, and documentation.

---

Critical Issues

1. Error Handling in Label Restoration

   • File: tekton/v1/tasks/ci/coderabbit-review-label.yaml, Lines: 56-58
   • Issue: The cleanup function attempts to restore the label if reconciliation fails but doesn't log the error or retry the restoration. This could leave PRs in an inconsistent state without visibility into the problem.
   • Suggestion: Log restoration errors explicitly and consider adding a retry mechanism with exponential backoff for label restoration.

     cleanup() {
       result=$?
       if [ "$restore" = true ]; then
         if ! add_label; then
           echo "Failed to restore label. Manual intervention required." >&2
           result=1
         fi
       fi
       exit "$result"
     }

2. Race Condition in Concurrent Task Runs

   • File: tekton/v1/tasks/ci/coderabbit-review-label.yaml, Lines: 128-130
   • Issue: The implementation notes potential race conditions when concurrent task runs attempt to reconcile the label state. However, no mechanism is in place to mitigate these conditions.
   • Suggestion: Introduce locking or transaction-like mechanisms to ensure atomic updates to the label. Alternatively, use GitHub's API conditional requests (e.g., If-None-Match headers) to detect conflicting updates.

---

Code Improvements

1. Validation of Environment Variables

   • File: tekton/v1/tasks/ci/coderabbit-review-label.yaml, Lines: 15-17
   • Issue: Regex validation for environment variables (PR_OWNER, PR_REPO, PR_NUMBER) is implemented but lacks logging for failed validations. Silent failures could make debugging difficult.
   • Suggestion: Add explicit error messages for failed validations.

     [[ "$PR_OWNER" =~ ^[A-Za-z0-9-]+$ ]] || { echo "Invalid PR_OWNER format"; exit 1; }
     [[ "$PR_REPO" =~ ^[A-Za-z0-9_.-]+$ ]] || { echo "Invalid PR_REPO format"; exit 1; }
     [[ "$PR_NUMBER" =~ ^[0-9]+$ ]] || { echo "Invalid PR_NUMBER format"; exit 1; }

2. Edge Case Handling for Review States

   • File: tekton/v1/tasks/ci/coderabbit-review-label.yaml, Lines: 88-90
   • Issue: The logic assumes reviews will always have predictable states, but does not handle unexpected states or missing data gracefully.
   • Suggestion: Add defensive checks and fallback handling for unrecognized review states or empty snapshots.

     state=$(jq -r '.review.state // "NONE"' <<< "$current")
     if [[ "$state" != "APPROVED" && "$state" != "CHANGES_REQUESTED" && "$state" != "DISMISSED" ]]; then
       echo "Unexpected review state: $state" >&2
       exit 1
     fi

---

Best Practices

1. Documentation Improvements

   • File: docs/guides/coderabbit-review-label.md
   • Issue: The guide provides detailed implementation notes but lacks a high-level overview and examples that would help users understand the workflow quickly.
   • Suggestion: Add a summary section describing the purpose and workflow of the label management, along with example scenarios (e.g., "What happens when CodeRabbit requests changes?" or "How does approval clear the label?").

2. Testing Coverage Gaps

   • File: tekton/v1/tasks/ci/coderabbit-review-label.yaml
   • Issue: The PR does not include unit or integration tests to validate the label reconciliation logic, particularly for edge cases like API failures, concurrent updates, or invalid review states.
   • Suggestion: Add automated tests using mock GitHub API responses to simulate various scenarios and ensure robustness.

3. Style Consistency in YAML Files

   • File: tekton/v1/triggers/templates/kustomization.yaml, Lines: 6-7
   • Issue: YAML formatting is inconsistent (e.g., inline comments vs. block comments). This could make maintenance harder over time.
   • Suggestion: Standardize formatting across YAML files with consistent comment styles and indentation.

---

Additional Notes

The PR description is empty, which makes it harder to understand the intent and scope of the changes. Adding a concise summary of the objectives and high-level implementation details would improve clarity for reviewers and stakeholders.

[ti-chi-bot]

I have already done a preliminary review for you, and I hope to help you do a better job.

Summary

This PR introduces a Tekton task and related configurations to enforce a do-not-merge/request-change label based on CodeRabbit AI review objections. The task ensures that PRs cannot be merged unless objections are cleared by a current-head approval. The implementation uses GitHub API calls and bash scripting for label management and validation. While the design is robust and the functionality appears well-implemented, there are several areas for improvement in error handling, readability, and adherence to best practices.

---

Critical Issues

1. Error Handling in Bash Script

   • File: tekton/v1/tasks/ci/coderabbit-review-label.yaml, lines 45–130
   • Issue: The script relies heavily on set -euo pipefail without granular error management, which may lead to unintentional termination. For example, if an API call fails due to a transient network issue, the entire task will exit without retry logic or fallback.
   • Solution: Add retry logic with exponential backoff for critical operations like API calls. Update the script as follows:

     retry() {
       local retries=3 delay=2
       for attempt in $(seq 1 $retries); do
         "$@" && return 0 || sleep $delay
         delay=$((delay * 2))
       done
       return 1
     }
     
     add_label() {
       retry gh api --method POST "$endpoint/issues/$PR_NUMBER/labels" -f "labels[]=$label" >/dev/null
     }

2. Race Condition on Label Restoration

   • File: tekton/v1/tasks/ci/coderabbit-review-label.yaml, lines 71–130
   • Issue: The restore=true flag and subsequent label restoration may conflict with another concurrent TaskRun that modifies the label state. This could lead to inconsistent outcomes.
   • Solution: Implement a lock or atomic verification mechanism to ensure label state consistency before restoration. Alternatively, use a webhook-driven approach to serialize tasks.

---

Code Improvements

1. Hardcoded Bot Username

   • File: tekton/v1/tasks/ci/coderabbit-review-label.yaml, line 78
   • Issue: The bot username coderabbitai[bot] is hardcoded, making it less flexible for deployment in environments with different bot configurations.
   • Solution: Pass the bot username as a parameter to the task. Update the script and task definition like so:

     params:
       - name: bot-username
         type: string

     bot_username="${params.bot-username}"
     .user.login == "$bot_username"

2. Magic Strings for Label Names

   • File: tekton/v1/tasks/ci/coderabbit-review-label.yaml, lines 46–130
   • Issue: The label do-not-merge/request-change appears multiple times as a hardcoded string. This increases the risk of errors if the label name changes in the future.
   • Solution: Define the label name as a single constant at the top of the script:

     label="do-not-merge/request-change"

3. Unverified Regular Expressions

   • File: tekton/v1/tasks/ci/coderabbit-review-label.yaml, lines 50–52
   • Issue: The [[ "$PR_OWNER" =~ ^[A-Za-z0-9-]+$ ]] and similar regex patterns could fail silently if the input is invalid.
   • Solution: Add explicit error handling to ensure invalid inputs are reported:

     if ! [[ "$PR_OWNER" =~ ^[A-Za-z0-9-]+$ ]]; then
       echo "Invalid PR_OWNER format: $PR_OWNER" >&2
       exit 1
     fi

---

Best Practices

1. Missing Documentation for Tekton Task Parameters

   • File: tekton/v1/tasks/ci/coderabbit-review-label.yaml, lines 7–11
   • Issue: The parameters owner, repo, and number lack descriptions in the task spec, making it harder for other developers to understand their purpose.
   • Solution: Add descriptions to each parameter:

     params:
       - name: owner
         type: string
         description: GitHub repository owner (e.g., "pingcap").
       - name: repo
         type: string
         description: GitHub repository name (e.g., "configs").
       - name: number
         type: string
         description: Pull request number.

2. Test Coverage for Edge Cases

   • File: Entire PR
   • Issue: The PR does not include automated tests for edge cases like dismissed reviews or failed API calls.
   • Solution: Add unit tests or integration tests to simulate scenarios like API failures, concurrent label modifications, and invalid review states.

3. Style Consistency

   • File: tekton/v1/tasks/ci/coderabbit-review-label.yaml, lines 45–130
   • Issue: The mix of single and double quotes in the script is inconsistent. For example, [[ "$PR_OWNER" =~ ^[A-Za-z0-9-]+$ ]] uses double quotes for variables, while 'CodeRabbit requested changes; blocking Tide.' uses single quotes for strings.
   • Solution: Standardize quotes to improve readability. Use double quotes for variables and single quotes for static strings.

4. Inline Comments in Bash Script

   • File: tekton/v1/tasks/ci/coderabbit-review-label.yaml, lines 45–130
   • Issue: While the script has some comments, the logic in functions like snapshot and can_clear is complex and could benefit from more detailed inline comments.
   • Solution: Add comments explaining each step of the logic.

---

Suggested Next Steps

1. Address critical issues related to error handling and race conditions.
2. Refactor the script to improve readability and maintainability.
3. Add documentation and tests to cover edge cases and make the task easier to understand and verify.
4. Consider using declarative YAML configurations for label management to reduce reliance on bash scripting.

[wuhuizuo]

Do we need to pay attention to these routine events?

[wuhuizuo]

consider to use the simple command:
gh pr view $PR_NUMBER --repo $OWNER/$REPO --json reviews --jq '[.reviews[] | select(.author.login == "coderabbitai[bot]" and .state == "CHANGES_REQUESTED")]'