Skip to content

Add protected branches configuration for main branch#642

Open
dave2wave wants to merge 7 commits intomainfrom
branch-protection
Open

Add protected branches configuration for main branch#642
dave2wave wants to merge 7 commits intomainfrom
branch-protection

Conversation

@dave2wave
Copy link
Copy Markdown
Member

As @potiuk suggested in #638

Signed-off-by: Dave Fisher <dave2wave@comcast.net>
@dave2wave dave2wave marked this pull request as ready for review March 31, 2026 15:20
@dave2wave dave2wave requested review from dfoulks1, potiuk and raboof March 31, 2026 15:21
Copy link
Copy Markdown
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would love others to agree.

Copy link
Copy Markdown
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but would be good to have an infra confirmation as well

@potiuk
Copy link
Copy Markdown
Member

potiuk commented Apr 1, 2026

i think we need to add some options, otherwise default doesnt enforce anything

Agree -> required_approving_review_count = 1 likely.

@dave2wave
Copy link
Copy Markdown
Member Author

Pulsar has some interesting notes about CI workflows these go with their .asf.yaml

Add required status checks and pull request review settings for the main branch.

Signed-off-by: Dave Fisher <dave2wave@comcast.net>
Copy link
Copy Markdown
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if we want to set require_code_owner_reviews, everything else looks good to me

@potiuk
Copy link
Copy Markdown
Member

potiuk commented Apr 2, 2026

We do not have CODEOWNERS here ? Do we ? So I guess require_code_owner_reviews: true is not a good idea?

Signed-off-by: Dave Fisher <dave2wave@comcast.net>
@dave2wave dave2wave requested a review from kevinjqliu April 2, 2026 13:09
Copy link
Copy Markdown
Member

@raboof raboof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm +1 on the concepts and on trying this out, though we should be ready to revert if it turns out it prevents the asfgit automated commits on this repo.

Copy link
Copy Markdown
Contributor

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Lets track to see if asfgit can still push to main after this is merged

@potiuk
Copy link
Copy Markdown
Member

potiuk commented Apr 4, 2026

LGTM!

Lets track to see if asfgit can still push to main after this is merged

Ah... It won't be able to - when branches are protected, bots cannot push to - it - that is a major blocker.

@potiuk
Copy link
Copy Markdown
Member

potiuk commented Apr 4, 2026

The only solution for that (and possibly a good one) would be that the fix creates a PR that we have to merge manually

@kevinjqliu
Copy link
Copy Markdown
Contributor

Looks like it depends on what asfgit's role/permissions are in this repo.

By default, the restrictions of a branch protection rule don't apply to people with admin permissions to the repository or custom roles with the "bypass branch protections" permission. You can optionally apply the restrictions to administrators and roles with the "bypass branch protections" permission, too. For more information, see Managing custom repository roles for an organization.

https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-protected-branches/about-protected-branches#about-branch-protection-rules

Could you run these to see if asfgit is an admin? I dont have permissions to run it:

gh api repos/apache/infrastructure-actions/collaborators/asfgit/permission
gh api orgs/apache/memberships/asfgit

Another possible solution is to allowlist asfgit explicitly with bypass_pull_request_allowances:
https://docs.github.com/en/rest/branches/branch-protection?apiVersion=2026-03-10#update-pull-request-review-protection

protected_branches:
  main:
    required_status_checks:
      strict: false
    required_pull_request_reviews:
      dismiss_stale_reviews: false
      required_approving_review_count: 1
      bypass_pull_request_allowances:
        users:
          - asfgit

@raboof
Copy link
Copy Markdown
Member

raboof commented Apr 4, 2026

The only solution for that (and possibly a good one) would be that the fix creates a PR that we have to merge manually

I think that would create too much work tbh

gh api repos/apache/infrastructure-actions/collaborators/asfgit/permission

I see this token apparently is indeed admin. perhaps we should consider switching to a non-admin token.

bypass_pull_request_allowances

That seems like a much nicer solution

@potiuk
Copy link
Copy Markdown
Member

potiuk commented Apr 4, 2026

That seems like a much nicer solution

Indeed - if it will work. I remember a change in GitHub that blanket-disabled bots from being able to bypass the "pull request approval" limitation of protected branches. but maybe this is a way to bypass it.

@dave2wave
Copy link
Copy Markdown
Member Author

I'll add this. Let's wait until Monday to merge and test,

      bypass_pull_request_allowances:
        users:
          - asfgit

Signed-off-by: Dave Fisher <dave2wave@comcast.net>
@dave2wave
Copy link
Copy Markdown
Member Author

Another possible solution is to allowlist asfgit explicitly with bypass_pull_request_allowances:
https://docs.github.com/en/rest/branches/branch-protection?apiVersion=2026-03-10#update-pull-request-review-protection

This fails because this feature is in the new apiVersion=2026-03-10 and apache/infrastructure-asfyaml is programmed for 2022 apiVersion.

@raboof
Copy link
Copy Markdown
Member

raboof commented Apr 5, 2026

Another possible solution is to allowlist asfgit explicitly with bypass_pull_request_allowances:
https://docs.github.com/en/rest/branches/branch-protection?apiVersion=2026-03-10#update-pull-request-review-protection

This fails because this feature is in the new apiVersion=2026-03-10 and apache/infrastructure-asfyaml is programmed for 2022 apiVersion.

filed apache/infrastructure-asfyaml#92

... but it's not a blocker for this PR since asfgit is currently still an admin token, if I understand correctly, right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants