Skip to content
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

Create rule which enforces the "Require status checks to pass before merging" branch protection rule in Github #49

Open
dmjb opened this issue Feb 7, 2024 · 3 comments
Labels
good first issue Good for newcomers P2 Nice to fix: non-critical items that should be evaluated and planned during issue triage

Comments

@dmjb
Copy link
Contributor

dmjb commented Feb 7, 2024

There does not appear to a rule for enforcing status checks in branch protections, e.g.

Screenshot 2024-02-07 at 16 39 22

Examining the Github API output for the branch protections shown in the screenshot, the relevant part of the response is:

  "required_status_checks": {
    "url": "https://api.github.com/repos/dmjb-test-org/action-test/branches/main/protection/required_status_checks",
    "strict": true,
    "contexts": [
      "A job to say hello"
    ],
    "contexts_url": "https://api.github.com/repos/dmjb-test-org/action-test/branches/main/protection/required_status_checks/contexts",
    "checks": [
      {
        "context": "A job to say hello",
        "app_id": 15368
      }
    ]
  }

Notes:

  1. If the Require status checks to pass before merging is unselected, then required_status_checks is set to null.
  2. The Require branches to be up to date before merging checkbox corresponds to the strict flag in the JSON object.
  3. It is possible to select these options without setting up a list of checks. Personal experience shows that this results in the checks not working (or at least not working consistently). We probably want to enforce that the Require status checks to pass before merging box is selected and the list of checks to be non-empty.
  4. We may want to create a separate rule forRequire branches to be up to date before merging or at least make it optional when creating a policy.
@jhrozek
Copy link
Contributor

jhrozek commented Feb 7, 2024

I think this is a great idea! If this is an interesting topic for you, then I think it would be nice to create these rules. Another reason I'm happy you brought this up is that we have rules that can flag a PR with a dependency with CVEs or with a low trusty score by failing a commit status on the PR but we currently don't have a way to enforce that the repo requires this commit status, so there's an extra manual step involved.

Feel free to ping me if you want a quick walkthrough on how the rules are implemented and some tips around debugging and creating a new rule - this would be a great thing to explore and write some documentation around, a lot of the how-to-write-a-new rule is really tribal knowledge and we should document it better.

@jhrozek
Copy link
Contributor

jhrozek commented Feb 7, 2024

btw for setting up the branch protection rules via a remediation, there's already a bespoke branch protection remediator that /should/ be able to set those settings up provided correct settings.

@evankanderson evankanderson added P2 Nice to fix: non-critical items that should be evaluated and planned during issue triage good first issue Good for newcomers labels Jul 16, 2024
@evankanderson
Copy link
Member

We still plan on doing this, and the description is a good set of acceptance criteria.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers P2 Nice to fix: non-critical items that should be evaluated and planned during issue triage
Projects
None yet
Development

No branches or pull requests

3 participants