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

Support for Branch Protection bypass_pull_request_allowances #249

Open
mike-stewart opened this issue Jul 25, 2022 · 9 comments
Open

Support for Branch Protection bypass_pull_request_allowances #249

mike-stewart opened this issue Jul 25, 2022 · 9 comments
Labels
enhancement New feature or request

Comments

@mike-stewart
Copy link

New Feature

Can safe-settings add support for the "Allow specified actors to bypass required pull requests" feature of required pull request reviews for branch protection?

  • Is the functionality available in the GitHub UI? If so, please provide a link to information about the feature.

Available in the UI as:

Allow specified actors to bypass required pull requests
Specify people, teams, or apps who are allowed to bypass required pull requests.

Screen Shot 2022-07-25 at 10 41 15 AM

Available in the API as "bypass_pull_request_allowances".

@mike-stewart mike-stewart added the enhancement New feature or request label Jul 25, 2022
@decyjphr
Copy link
Collaborator

Thanks, will add this in the coming weeks.

@dan-ih
Copy link

dan-ih commented Sep 6, 2022

Any progress on this @decyjphr ?

@ghost
Copy link

ghost commented Oct 27, 2022

I would also be interested to know when this is coming out :)

@viks-confluent
Copy link

@decyjphr any update on this would be appreciated. Thanks

@martinm82
Copy link
Contributor

martinm82 commented Nov 11, 2022

Aren't the attributes out of the box available in safe-settings?

Also should actually all the boolean parameters available in the API (https://docs.github.com/en/rest/branches/branch-protection#update-branch-protection) be added to the reformat function? (e.g. allow_force_pushes, block_creations, lock_branch, allow_fork_syncing)

@ghost
Copy link

ghost commented Nov 12, 2022

@mike-stewart @dan-ih @viks-confluent It seems @decyjphr has already implemented this 😄!
So, i did a little bit of test and yes, the attributes are already available it's just that it is not mentioned in the documentation/examples, here is how i used them and it worked:
By defining:

bypass_pull_request_allowances:
          users: [ "GranitFazliuRB","User2","User3"]

under the required_pull_request_reviews section

Only tested it with users but it should work with teams and apps arrays as well

branches:
  - name: default
    protection:
      required_pull_request_reviews:
        required_approving_review_count: 1
        dismiss_stale_reviews: true
        require_code_owner_reviews: true
        bypass_pull_request_allowances:
          users: [ "GranitFazliuRB","User2","User3"]        
      required_status_checks:
        strict: true
        contexts: []
      enforce_admins: true
      restrictions:
        apps: []
        users: []
        teams: []

@anderssonjohan
Copy link
Contributor

related PR #419

@anderssonjohan
Copy link
Contributor

Aren't the attributes out of the box available in safe-settings?

Also should actually all the boolean parameters available in the API (docs.github.com/en/rest/branches/branch-protection#update-branch-protection) be added to the reformat function? (e.g. allow_force_pushes, block_creations, lock_branch, allow_fork_syncing)

@martinm82 Ah! So that may be the reason why block_creations always is identified as changed all the time?

@martinm82
Copy link
Contributor

Aren't the attributes out of the box available in safe-settings?
Also should actually all the boolean parameters available in the API (docs.github.com/en/rest/branches/branch-protection#update-branch-protection) be added to the reformat function? (e.g. allow_force_pushes, block_creations, lock_branch, allow_fork_syncing)

@martinm82 Ah! So that may be the reason why block_creations always is identified as changed all the time?

I haven't checked that but the reason why all these params work out of the box is because we use the REST API format of the payload which gets used 1:1.

P.S. We will run soon into limitations with the REST API. For example one cannot create branch protection rules and use wildcards in the branch pattern which is only supported by GraphQL API.

torgst added a commit to helse-sorost/safe-settings that referenced this issue Sep 14, 2024
I found the logic behind some of what happened here hard to follow and updated the comments to try to make it easier to follow.
torgst added a commit to helse-sorost/safe-settings that referenced this issue Sep 14, 2024
I found the logic behind some of what happened here hard to follow and updated the comments to try to make it easier to follow.
torgst added a commit to helse-sorost/safe-settings that referenced this issue Sep 14, 2024
I found the logic behind some of what happened here hard to follow and updated the comments to try to make it easier to follow.
decyjphr added a commit that referenced this issue Sep 16, 2024
* update comments from #249

I found the logic behind some of what happened here hard to follow and updated the comments to try to make it easier to follow.

* fix environments updating global array

The environments plugin was changing `MergeDeep.NAME_FIELDS`, which is a global object. The reason was to avoid environments being filtered out from the change list if they only have a name field.
However, the environments plugin has it's own overriden sync method, and thus we can simply drop the whole filtering from that method.

Fixes #108

* remove repeating line

This has to be a "typo" from 9a74e05 calling the same assignment twice. Removing to clean up.

---------

Co-authored-by: Torgeir S. hos Sykehuspartner <[email protected]>
decyjphr pushed a commit that referenced this issue Sep 16, 2024
* update comments from #249

I found the logic behind some of what happened here hard to follow and updated the comments to try to make it easier to follow.

* fix environments updating global array

The environments plugin was changing `MergeDeep.NAME_FIELDS`, which is a global object. The reason was to avoid environments being filtered out from the change list if they only have a name field.
However, the environments plugin has it's own overriden sync method, and thus we can simply drop the whole filtering from that method.

Fixes #108

* remove repeating line

This has to be a "typo" from 9a74e05 calling the same assignment twice. Removing to clean up.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants