Preview Feedback: Require review from specific team rule #178776
Replies: 25 comments 54 replies
-
|
Great in theory, but it doesn't seem negation works right now, when it says it uses the same syntax as .gitignore files! I.e. this does not work: PRs are still blocked + require specific approval if a negated/allowed file is edited. |
Beta Was this translation helpful? Give feedback.
-
|
I haven't had the chance to try this feature yet, since it's still in preview, and I would need to ask my organization manager to enable it. Before asking them, I have a question though. It seems unclear from the announcement what the exact behaviour is for pull requests. Is it similar to how CODEOWNERS work? Specifically I'm curious if this feature addresses https://github.com/orgs/community/discussions/178776. We would really like to be able to assign/require a team as reviewer, so that the team is automatically added to any PR as specified by the ruleset, but keep the team there as individual members add reviews to the PR. |
Beta Was this translation helpful? Give feedback.
-
|
@patrick-knight should this be supported in the REST API? |
Beta Was this translation helpful? Give feedback.
-
|
Great feature, our team is migrating from Azure DevOps and this covers a gap that codeowners didn't quite fill. One issue is that for repository rulesets, I am only able to setup teams that have been given direct access to the repository. Teams with organization access to all repositories are not selectable. This limitation isn't there for rulesets created at the organization level. |
Beta Was this translation helpful? Give feedback.
-
|
Hey @patrick-knight - as you can see I've been interested in this for quite a while. After doing some testing, I think the current preview has potential but has some limitations compared to Specifically there are some cases where we require an approval for some paths for either team A or team B in CODEOWNERS Also the Extending on this - the functionality is still locked within |
Beta Was this translation helpful? Give feedback.
-
|
There doesn't seem to be any UI on the PR itself showing that this review is needed. Is this expected behaviour, or is my ruleset not being applied appropriately? |
Beta Was this translation helpful? Give feedback.
This comment was marked as spam.
This comment was marked as spam.
-
|
What do we need to do to be able to test this preview feature in our organization or repo? I am not seeing it available to enable/disable in my account under the "Feature preview dialog," nor do I see I way to request access to a technical preview.... I am a repo admin, but not an org admin if that matters. We very much are looking forward to this feature. Thanks! |
Beta Was this translation helpful? Give feedback.
-
|
@patrick-knight While not having the same functionality, we could control some exclusion logic with the order of patterns in CODEOWNERS, is that still true? Here is my scenario: In CODEOWNERS I just had Team2 and Team3 at the top with the "*": |
Beta Was this translation helpful? Give feedback.
-
|
Hi all :-) |
Beta Was this translation helpful? Give feedback.
-
|
Love this feature. We've been looking for a way to decouple ownership from review required. I would like to humbly request an increase to the limit of 15 filter patterns per required reviewer. We're working with a pretty large monorepo and require quite granular rules. At the moment the highest number of patterns is 26 for one required reviewer but that may increase in the future. Thanks for the amazing work on this feature. |
Beta Was this translation helpful? Give feedback.
-
|
Subteams don't show up on the list of teams, which largely defeats the utility for us. If we have a team |
Beta Was this translation helpful? Give feedback.
-
Beta Was this translation helpful? Give feedback.
-
|
Hey @patrick-knight Love the addition of this new feature. This is something we have wanted since switching to GitHub. Just discovered it yesterday. We have a custom action running on every PR currently that solves the problem this addresses, Unfortunately we have one use case that is not supported. Please add the ability to not request a review from the required team. Sounds strange but it would be a very useful setting. With AI, now non-engineers could write code and open Pull Requests. But we would want to require a Engineer reviews every PR before they can be merged. We have a github team that contains all the engineers at the company, but if you use this setting it will ping everyone in the company, which is way too noisy. There is other uses for this too we would want to use it for, but this is one I imagine we might share with other companies. ( Side note: I also noticed that a team with round robin autoassignment and the I just want a toggle that lets me not request a review from a team, but still require it. That is the biggest blocker for us being able to use this currently. Thanks again for implementing this feature. |
Beta Was this translation helpful? Give feedback.
-
|
@patrick-knight If possible it would be great if there could be a different UI treatment that informs users when review is required by a particular team. For context in our current setup teams own many files via the CODEOWNERS file and are required reviewers on a subset of those files via Rulesets. When an owned file is modified the owner is added as a reviewer alongside the "shield with keyhole" icon. There is no indication in the assigned reviewers section if their review is blocking or not based on the file matching the Rulesets. There is an indicator in the ExampleTeam Team |
Beta Was this translation helpful? Give feedback.
-
|
First off: Thank you for doing this 🙏🏼 A great feature of CODEOWNERS is that it warns you that you're about to request a review from a team, but does NOT do so until you mark the PR as ready to review. To build on what @gabrieltaylor said, I'd suggest to augment the UI as this:
|
Beta Was this translation helpful? Give feedback.
-
|
I'm currently implementing this for my team and I find that the ruleset being in the settings is not very user friendly, especially in terms of discoverability. Most environments where this feature is useful do not grant users access to the settings. Therefore, I'm currently working on a code-as-configuration design like this: # .slices/ci.yml:
description: Continuous Integration Workflows
reviewers:
admins: ~
file_patterns:
- '.github/workflows/ci-*.yml'# .slices/javascript.yml:
description: JavaScript Files
reviewers:
javascript-core-team:
minimum_approvals: 1
frontend-team: ~
review_bypass:
admins: pull_request
file_patterns:
- '**/*.js'
- '**/*.ts'Upon a push to Additionally, it automatically applies Why Slices?
This is probably overkill for the feature you're building, but I thought I'd share. The code-as-configuration is a big winner IMO. |
Beta Was this translation helpful? Give feedback.
-
|
Is there anything on the roadmap for implementing something similar for statuses? I'm in the processes of moving from Azure Repos to Github. Branch protection rules in Azure Repos offer path filtering both for assignment of required reviewers but also for requiring specific status for matching paths. |
Beta Was this translation helpful? Give feedback.
-
|
Suggestion: File patterns are a very powerful way to control rulesets. I would suggest moving the concerns from the "Require a pull request before merging > Require review from specific teams" section and to the root of the ruleset. Reasons:
|
Beta Was this translation helpful? Give feedback.
-
|
I would love this feature if it wouldn't automatically add the team as a reviewer – especially when I've already added someone from that team as a reviewer directly. What we're after is basically a "Review in this repo required from this team" but we don't want the whole team to notified or have their review inbox filled up with team review requests. Our workflow is along these lines - we have multiple teams in our product, and each team is has 1 or more repositories they work with. Repository/Code ownership is at the team level - so let's say Team A owns Repo A and Team B owns Repo B. Sometimes member of Team A works in Repo B, and so someone from Team B should review that pull request, we just add a specific team member or two from that team to have the code reviewed. While this feature gets us part of the way there - as in we're able to enforce a review of the code in Repo B by a member of Team B - the feature will add the team as a reviewer to the PR, even though a member of Team B was already explicitly added to the PR.
At the moment it feels like some parts of this feature is "CODEOWNERS but configured from a Ruleset", the CODEOWNERS feature has some of the same problems, so I'm guessing this probably uses a lot of the same stuff under the hood, so solving/improving this may benefit both of these features :-) |
Beta Was this translation helpful? Give feedback.
-
|
Is this meant to be available at the enterprise level? For my enterprise branch policy I have these options:
only when creating a new org-level policy do I see the setting for specific team review:
|
Beta Was this translation helpful? Give feedback.
-
|
@patrick-knight This is a great start! Specifically, for a given path, I want to be able to skip required reviews when the author is a member of a specific team, while still requiring reviews for everyone else. I see this idea touched on above, but not as its own distinct request: Concrete example (based on @dtschiffman's comment above): Today, “require reviews from a specific team” is effectively one-dimensional: it can enforce who must review, but it can’t express who is exempt from review requirements. This request is about adding that missing dimension. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for this great feature. I have a question about this rule in combination with "Dismiss stale pull request approvals when new commits are pushed". Is it possible to only require an additional review if the target files have been updated? Example:
Is it possible to retain the infra team approval unless further main.tf updates are pushed. |
Beta Was this translation helpful? Give feedback.
-
|
Quick update. We're going to mark this as GA next week to note it's a stable feature. Doesn't mean you shouldn't keep sending feedback our way. Y'all helped up find the sub-team issues and helped champion the |
Beta Was this translation helpful? Give feedback.
-
|
Is there a plan to have this taken into accout in the PullRequestReviewDecision? All other approval related rules are showing as REVIEW_REQUIRED if they are not fulfilled, but this is not the case for the "Require review from specific team" rule. |
Beta Was this translation helpful? Give feedback.













Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
-
Required review by specific teams now available as a preview feature in rulesets
Take control of your protected branches! You can now use rulesets to require approvals from specific teams for changes to specific files and folders.
What's new
Rulesets vs. CODEOWNERS
While CODEOWNERS is great for defining ownership, this new ruleset focuses on policy enforcement. It makes it simple to require specific approvals on sensitive branches and critical code paths, all while scaling seamlessly across your enterprise.
This new rule is designed to augment CODEOWNER files but not replace them. The CODEOWNERS files will continue to be the way to manage ownership, support individuals as reviewers, and request reviews even if not required.
🌟 Leave a comment!
What do you think? Join the discussion and leave your feedback below!
Beta Was this translation helpful? Give feedback.
All reactions