[WIP] CI PRs autoclean

[whoisarpit]

PR Checklist

• [ ] The commit message follows our guidelines: Code of conduct
• [ ] Tests for the changes have been added (for bug fixes / features)
• [ ] Docs have been added / updated (for bug fixes / features)
• [ ] Does this PR introduce a breaking change?
• [ ] Include PR in release notes?

PR Type

• [ ] Bugfix
• [ ] Feature
• [ ] Refactoring
• [ ] Build /CI
• [ ] Documentation
• [ ] Others

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Other information

[patched-codes[bot]]

Code Review: CI-related changes for Automatic PR and Branch Management

Summary

This review examines the recent CI-related changes aimed at automating the cleanup of stale branches and PRs in the repository. While automation is beneficial for maintaining a clean codebase, there are several areas where improvements can be made to enhance security, maintain consistency, and ensure smooth workflows.

1. Potential Security Vulnerability in Branch Deletion

File Changed: .github/workflows/close-stale-ci-prs.yml

Details:

• Security Issue: The current configuration utilizes the default GITHUB_TOKEN, which may not possess sufficient permissions to delete branches securely.
• Recommendation: Implement a dedicated token with the necessary branch deletion permissions. Clearly document required permissions in the workflow file to avoid accidental misuse.

Affected Code Snippet:

- name: Delete stale branches
  env:
    GH_TOKEN: $https://github.com/patched-codes/patchwork/pull/1601/files#diff-58d9f1be2f7d02184c9a17a9d8e1f2d025c03249f4da9008f2967a4f66c1d3c1

Start Line: 38 End Line: 63

2. Inconsistent Branch Prefix Format

File Changed: .github/workflows/test.yml

Details:

• Issue: There's an inconsistency in the branch prefix format.
  • The affected snippet uses --branch_prefix=patchwork-ci-generatereadme.
  • Other snippets include a trailing dash and unique identifiers (e.g., --branch_prefix=patchwork-ci-<operation>-).
• Impact: This inconsistency may lead to naming conflicts during parallel operations.
• Recommendation: Standardize the branch prefix format across all CI commands incorporating a unique identifier and a trailing dash.

Affected Code Snippet:

--branch_prefix=patchwork-ci-generatereadme \

Start Line: 236 End Line: 236

3. Security Considerations for Automated PR Closures

Details:

• Threshold Issue: The current configuration aggressively closes PRs that remain open for more than 1 hour.
  • This may be insufficient if reviews or CI processes extend beyond an hour.
• Recommendation: Extend the closure threshold or add conditions based on CI status or specific labels to ensure PRs are only closed when truly stale.

Affected Code Snippet:

if [ $age_hours -gt 1 ]; then
  echo "Closing PR #$pr_number (age: $age_hours hours)"

Start Line: 35 End Line: 39

---

Conclusion

These changes are aimed at improving workflow efficiency by automatically managing branches and PRs. However, attention to the highlighted security vulnerabilities, consistency in branch naming conventions, and additional considerations in timing thresholds will contribute to a more robust and reliable CI process.

Kindly review the recommendations and consider implementing the suggested improvements for a smoother development experience. Feel free to reach out if there are any questions or further discussions needed.

---

Note: This review aims to foster a collaborative code review process. Feedback and suggests changes in a constructive and actionable manner for continuous improvement.

[patched-codes[bot]]

Code Review Summary

Affected File: .github/workflows/close-stale-ci-prs.yml

1. Syntax Errors in AWK Commands:

• Details: The code contains syntax errors in the awk command usage that will cause the GitHub Action to fail.
• Affected Code Snippet:

  pr_number=$(echo "$line" | awk \.'{| .print $1}\\')
  created_at=$(echo "$line" | awk \.'{| .print $2}\\')
  

• Lines: 26 to 27
• Details: Similarly, syntax errors are present in Delete stale branches section.
• Affected Code Snippet:

  branch_name=$(echo "$line" | awk \.'{| .print $1}\\')
  created_at=$(echo "$line" | awk \.'{| .print $2}\\')
  

• Lines: 52 to 53

2. Aggressive PR Closing Policy:

• Details: The GitHub action uses a 1-hour stale period for PRs, which might be too aggressive and could accidentally close valid PRs.
• Affected Code Snippet:

  if [ $age_hours -gt 1 ]; then
    echo "Closing PR #$pr_number (age: $age_hours hours)"
    gh pr comment $pr_number --body "This PR has been automatically closed because it's been open for more than 1 hour. This is a CI-generated PR and should be reviewed and merged promptly if valid."
    gh pr close $pr_number
  fi
  

• Lines: 32 to 36

3. API Rate Limits Handling:

• Details: The script does not manage potential API rate limits from GitHub, potentially causing the action to fail when processing many branches/PRs.
• Affected Code Snippet:

  # Get all open PRs with patchwork-ci- branches
  prs=$(gh pr list --state open --json number,headRefName,createdAt --jq \'.[] | select(.headRefName | startswith("patchwork-ci-")) | "\\(.number) \\(.createdAt)"\\')
  

• Lines: 20 to 21

4. Security Issue with Branch Deletion:

• Details: The script does not verify ownership of branches prior to deletion, posing a risk of deleting important branches that match the naming pattern.
• Affected Code Snippet:

  if [ $age_days -gt 7 ]; then
    echo "Deleting branch $branch_name (age: $age_days days)"
    gh api -X DELETE repos/$https://github.com/patched-codes/patchwork/pull/1601/files#diff-1305b58fd18285bebf62e6be93b3ad2c2f3cba298e7145e316ab979da343b023/git/refs/heads/$branch_name
  fi
  

• Lines: 56 to 59

---

Affected File: patchwork/patchflows/GenerateREADME/GenerateREADME.py

1. Inconsistency in Checking Input Keys:

• Details: The file checks for pr_title but does not verify branch_prefix, creating inconsistency with other modules.
• Code Snippet Showing Issue:

  if "pr_title" not in final_inputs.keys():
      final_inputs["pr_title"] = f"PatchWork {self.__class__.__name__}"
  

• Recommendation: Add the missing branch_prefix check to maintain consistency across the codebase.

  if "branch_prefix" not in final_inputs.keys():
      final_inputs["branch_prefix"] = f"{self.__class__.__name__.lower()}-"
  

---

Kindly make the necessary updates and address the highlighted issues to ensure consistent functionality across the codebase.

[whoisarpit]

not relevant anymore