-
Notifications
You must be signed in to change notification settings - Fork 155
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
Feature: Allow PR status checks to be managed external to safe settings #741
base: main-enterprise
Are you sure you want to change the base?
Feature: Allow PR status checks to be managed external to safe settings #741
Conversation
…rigger top level branch protection rules across all repos. Make a clone of the branch settings before modifying it since it's a shared object.
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.
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
lib/plugins/branches.js:53
- Redundant check for 'protection.allow_deletions'. It should be simplified to 'protection.allow_deletions = protection.allow_deletions && protection.allow_deletions.enabled'.
protection.allow_deletions = protection.allow_deletions && protection.allow_deletions && protection.allow_deletions.enabled
Tip: Leave feedback on Copilot's review comments with the 👎 and 👍 buttons to help improve review quality. Learn more
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.
@jitran , Thanks for the detailed work on this feature! I’ve reviewed the changes, and overall, this is a solid implementation that brings much-needed flexibility to managing PR status checks. That said, I have a few suggestions to improve error handling, documentation clarity, and test coverage:
-
In
branches.js
andrulesets.js
, there are scenarios where API failures (e.g., 500-series errors) are logged but not surfaced to the user. Consider adding retry logic for transient errors and ensuring failures are gracefully handled with actionable error messages. -
Include a warning about potential desync when rulesets are modified directly via the GitHub UI.
-
In
overrides.js
, it would be good to add safeguards in removeOverrides to handle malformed configurations or unexpected data structures more robustly. -
Add an example showing how {{EXTERNALLY_DEFINED}} interacts with hierarchical rulesets (org, repo, and sub-org levels).
Let me know your thoughts on these suggestions!
Nice job!
Background
There have been a number of requests from users who have implemented branch protection rules and rulesets across their organisations, but would like their PR status checks to be defined outside of safe settings.
When these rules are defined at the repository or suborg level, it's not so bad, as the safe settings PR status checks only affect a subset of repositories. Users have a bit more control over their safe settings configuration.
When the rules are defined at the org level, it's a bit more problematic, as the safe settings PR status checks affect all repositories in the org.
Changes
{{EXTERNALLY_DEFINED}}
in the status checks for their branch protection rules or rulesets, safe settings will not revert status checks in GitHub.Tests
Ensured all unit tests pass.
Manual testing with rulesets (repo, suborg, and org levels)
The following was tested in sequential order:
{{EXTERNALLY_DEFINED}}
defined in the status checks{{EXTERNALLY_DEFINED}}
in the list{{EXTERNALLY_DEFINED}}
from the existing ruleset's status checks in safe settingsManual testing with branch protection rules (repo, suborg, and org levels)
The following was tested in sequential order:
{{EXTERNALLY_DEFINED}}
in the status checks, and the second rule had two status checks defined