Ruleset - rules with bypass group still applied to them in case of PR creation

[marcaurele]

Select Topic Area

Product Feedback

Body

Hi,

I'm playing with the repository rulesets as it's looking more flexible that standard PR branch protection. I have a feedback on the bypass system.

I configured 2 rules targeting the default branch:

• rule A:
  • enforce CI check,
  • deny force push,
  • deny deletion
• rule B:
  • require PR before merging
    • require review from CODEOWNERS
  • bypass list:
    • ci-bot: Always

I was expecting that a our bot user in the ci-bot group would be able to still create a PR, but the rule B will not apply as it's part of the bypass list, so the rules would not enforce the PR review count and CODEOWNERS.

I also confirm that I do not have a "Branch protection rule" in place in the older settings of the repository which could collide with the rulesets.

While I see, after testing, it is more about the bypass group which does not need to create a PR, but if they do, the rules will apply to them... Why was it design so? I expected that the bypass group is really a group of users for which the content of the rule is not applied.

[marcaurele]

I believe having the implementation changed, to really not apply the rule to the group in the bypass groups would as well solve the case described here: https://github.com/orgs/community/discussions/68271 with repository's CODEOWNERS not needing to get approval.

[appatalks]

👋 Hello there!

expecting that a our bot user in the ci-bot group would be able to still create a PR, but the rule B will not apply as it's part of the bypass list, so the rules would not enforce the PR review count and CODEOWNERS.

I had recently ran into a similar report, and we had discovered that in the one case, legacy branch protections were alongside being deployed, where legacy branch protection does not support bypass lists for every rule. So, if the bot was added to the bypass list underneath the PR rule, this would not apply to status checks.

We recommend customers move their branch protections to rulesets if they want the full bypass experience.

Warm regards,
AppaTalks

[jeremy-daley-kr]

Even if there is no branch protection rule, it is not possible for us to bypass code owners. At least with MergeQueues enabled.

@secustor It is actually, but with a fair number of workaround steps... Use whichever language you'd like, but mine is in Node.

1. Create an app/bot with elevated permissions. Mine has god-ish permissions, so I'm not sure which exactly are required for this tbh.
2. Create 2 rulesets. One with the majority of requirements (status checks, merge queue, etc.). The other exclusively for codeowner enablement.
3. Make the app/bot your bypass actor for the codeowner ruleset, and set to "Exempt".
4. Create a dispatchable workflow with a PR number input.
5. Acquire a server-to-server token for the app to make some API calls
6. First call is just a preliminary check to verify the "actor" permissions of the dispatch user are "admin":

  const { data: permission } =
    await octokit.rest.repos.getCollaboratorPermissionLevel({
      owner,
      repo,
      username,
    });

  return permission.permission === 'admin';

7. Lastly, use this API/graphql to enqueue your PR to merge queue on behalf of the app/bot.

  const pr = await octokit.rest.pulls.get({
    owner,
    repo,
    pull_number: prNumber,
  });

  if (pr.data.state !== 'open') {
    throw new Error(`PR #${prNumber} is not open (state: ${pr.data.state})`);
  }

  // Use GraphQL to add PR to merge queue
  const query = `
    mutation EnqueuePullRequest($pullRequestId: ID!) {
      enqueuePullRequest(input: {
        pullRequestId: $pullRequestId,
        expectedHeadOid: "${pr.data.head.sha}"
      }) {
        mergeQueueEntry {
          id
          position
          state
        }
      }
    }
  `;

  await octokit
    .graphql(query, {
      pullRequestId: pr.data.node_id,
    })
    .catch((error) => {
      throw new Error(
        `Failed to send PR #${prNumber} to merge queue: ${error.message}`,
      );
    });

[secustor]

Thanks for the write up, but Admin users are sadly not an option for us.

Your write more or less confirms the CODEOWNERS feature simply has no awareness of the rules system and configures itself based on this.

We are evaluating right now if we can move from CODEOWNERS to centrally via Terraform managed Require review from team configuration.

[jeremy-daley-kr]

It certainly doesn't appear to have the type of integration that I'd expect, but with rulesets, CODEOWNERS is itself a rule. When you stack the rulesets, a bypass actor can be completely exempt from that whole ruleset containing the CODEOWNERS rule, hence the creation of 2 separate rulesets.

You don't necessarily have to have an admin user... just an app with the necessary permissions to call the API graphql. That's really the sauce.

[secustor]

We have up to 8 rulesets stacked in my org and we bypass the merge queue for example.

Back then it definitely did not work with Code owners and multiple "required PR" rulesets.

Though this could have changed in the meantime

[jeremy-daley-kr]

Gotcha. I should also highlight that "Exempt" is also the important bit. "Always" and "Allow for Pull Requsts Only" doesn't give you the same checkbox to bypass in the PR that you'd get for non-merge queue. That had me stumped. The reason I suggested an app to make "Exempt" is because we actually don't want even our admins to be exempt... I actually want to know when I need more approvals.

Anyway, I still think they need to clear things up a bit on that. I even submitted this a while back:
https://github.com/orgs/community/discussions/147620