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

Mention that "Allow specified actors to bypass required pull requests" only applies to repo admins #24726

Closed
wants to merge 3 commits into from

Conversation

dhermes
Copy link

@dhermes dhermes commented Mar 28, 2023

Why:

Closes: #24725

What's being changed (if available, include any code snippets, screenshots, or gifs):

Phrasing a user reference as "repository admin users" instead of "actors"

Check off the following:

  • I have reviewed my changes in staging (look for the "Automatically generated comment" and click the links in the "Preview" column to view your latest changes).
  • For content changes, I have completed the self-review checklist.

@welcome
Copy link

welcome bot commented Mar 28, 2023

Thanks for opening this pull request! A GitHub docs team member should be by to give feedback soon. In the meantime, please check out the contributing guidelines.

@github-actions github-actions bot added the triage Do not begin working on this issue until triaged by the team label Mar 28, 2023
@dhermes
Copy link
Author

dhermes commented Mar 28, 2023

I'm just trying to start a conversation and figured this PR (+ issue) would be a good place to start.

From looking at the feature, it appears the goal is for it to work for non-admin users. So maybe it's actually a bug in GitHub. Some potential bugs with the product that might be issued:

  • (A) Non-admin users should be able to "bypass required pull requests"
  • (B) Non-admin users should not be able to be added to the "Allow specified actors to bypass required pull requests" list
  • (C) There should be a warning if an (invalid) non-admin users is added to / present in the "Allow specified actors to bypass required pull requests" list

@github-actions
Copy link
Contributor

github-actions bot commented Mar 28, 2023

Automatically generated comment ℹ️

This comment is automatically generated and will be overwritten every time changes are committed to this branch.

The table contains an overview of files in the content directory that have been changed in this pull request. It's provided to make it easy to review your changes on the staging site. Please note that changes to the data directory will not show up in this table.


Content directory changes

You may find it useful to copy this table into the pull request summary. There you can edit it to share links to important articles or changes and to give a high-level overview of how the changes in your pull request support the overall goals of the pull request.

Source Preview Production What Changed
repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule.md fpt
ghec
ghes@ 3.8 3.7 3.6 3.5 3.4
ghae
fpt
ghec
ghes@ 3.8 3.7 3.6 3.5 3.4
ghae

fpt: Free, Pro, Team
ghec: GitHub Enterprise Cloud
ghes: GitHub Enterprise Server
ghae: GitHub AE

@cmwilson21
Copy link
Contributor

cmwilson21 commented Mar 30, 2023

@dhermes Thank you for opening a PR and linking it to your issue! ✨

We will take a look at the docs update, but if you have any suggestions or feedback for the feature, that conversation would best fit over at community discussions. 💖

@cmwilson21 cmwilson21 added content This issue or pull request belongs to the Docs Content team waiting for review Issue/PR is waiting for a writer's review pull requests Content related to pull requests repositories Content related to repositories and removed triage Do not begin working on this issue until triaged by the team pull requests Content related to pull requests labels Mar 30, 2023
@dhermes
Copy link
Author

dhermes commented Mar 30, 2023

Thanks @cmwilson21, I started https://github.com/orgs/community/discussions/51495 as well with my "potential bug" comment.

@elee-msft
Copy link

I would like to emphasize that the "Allow specified actors to bypass required pull requests" feature appears to be intended to grant bypassing privileges to accounts that aren't otherwise admins in the repo, particularly for apps like the Azure Pipelines app, which is the scenario I need and what I'm currently blocked on. If the purpose was simply to grant bypass privileges to admins, they already have that by default unless the "Do not allow bypassing the above settings" setting is turned on. So someone needs to check with the product team about what the design intent actually is. :-)

@isaacmbrown isaacmbrown self-assigned this May 25, 2023
@isaacmbrown
Copy link
Contributor

isaacmbrown commented May 25, 2023

Hi @dhermes, thanks for the issue and linked PR!

Could you share more context about the issue you're experiencing and how we'd reproduce it?

From a quick test, I was able to add a user with just write access as an actor with bypass permissions. As I'd expect, I wasn't able to add a user with just read access.

The only problem I found was that I had to enter almost the entirety of the user's name before they come up in the list of suggestions.

Is the specific issue here:

  • You've found you can't add users with just write access (this sounds like a bug if so)
  • You are trying to add a Team or App, but you can't
  • You can add the user to the branch protection rule, but they don't actually get permission to push without a PR?

@dhermes
Copy link
Author

dhermes commented May 25, 2023

Could you share more context about the issue you're experiencing and how we'd reproduce it?

@isaacmbrown

Repo settings

Add a user to a repo with "Write" permissions (crucial: this user is not an org admin)

Screenshot 2023-05-25 at 11 55 40 AM

Branch protection rule

Add that user to the "Allow specified actors to bypass required pull requests" list

Screenshot 2023-05-25 at 11 54 28 AM

PR experience (from perspective of user)

The user cannot bypass on a given PR, for example:

Screenshot 2023-05-25 at 11 51 26 AM

This should say

Merge without waiting for requirements to be met (bypass branch protections)

Screenshot 2023-05-25 at 12 01 19 PM

@isaacmbrown
Copy link
Contributor

isaacmbrown commented May 25, 2023

Hi @dhermes, I will look closer, but the restriction there looks to be to do with the required code owner review?

I believe selecting the bypass option should allow the selected user to push to the protected branch without creating a pull request at all. I don't believe it's meant to exempt them from the rest of the rules once a pull request is created.

As far as I know the bypass option on a pull request is just tied to your repo admin rights, and you don't need to add a user to the list specifically.

@isaacmbrown
Copy link
Contributor

I got confirmation on this: #24726 (comment)

@elee-msft, was the problem you were having the same thing @dhermes mentioned above? Or were you having a different issue that would require a docs update?

@elee-msft
Copy link

@isaacmbrown My issue was a little different. I'm trying to configure things so that the GitHub “Azure Pipelines” app can push directly to main, bypassing the branch rules that would normally forbid that. (The pipeline is attempting to sync code between an ADO repo and a GitHub repo). I opened a feedback item on this and they pointed me to this docs issue, saying that the bypass option is applied to the “repository admin users” instead of “actors”. That doesn't make sense to me from the plain reading of the UI, but if that's indeed supposed to be the desired design, then the docs should be updated to clarify that. But I still think the current behavior is actually a bug and I'm hoping someone can confirm that with the product team.

@isaacmbrown
Copy link
Contributor

Hi @elee-msft, thanks for getting back to me! I got confirmation from product that the feature is intended to work as described. Therefore I'd suggest contacting GitHub support -- but first, just want to check one thing with you:

In the thread you linked, you listed your branch protection rules:

a) Require a pull request before merging
b) Require approvals
c) Require review from Code Owners
d) Allow specified actors to bypass required pull requests, with the Azure Pipeline app selected (screenshotted in my previous reply)
e) Require status checks to pass before merging
f) Do not allow bypassing the above settings
g) Restrict who can push to matching branches

Is the Azure Pipeline app also given permission to push to the protected branch in g)? If not, that rule would also be preventing the app from pushing to the protected branch, irrespective of the pull request rules.

@isaacmbrown
Copy link
Contributor

Based on the discussion above I don't think a docs update is required here, so I'm closing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content This issue or pull request belongs to the Docs Content team repositories Content related to repositories waiting for review Issue/PR is waiting for a writer's review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The "Allow specified actors to bypass required pull requests" setting only applies to repo admins
4 participants