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

Option to require all changed files be evaluated by a rule #547

Open
robstolarz opened this issue Mar 9, 2023 · 2 comments
Open

Option to require all changed files be evaluated by a rule #547

robstolarz opened this issue Mar 9, 2023 · 2 comments

Comments

@robstolarz
Copy link

Hi folks, is there a way to / would y'all consider an option to require all changed files be evaluated by at least one rule? We think something like this would make for a great forcing function to encourage all teams to own the files they're changing, and it would help if the tool that ensured review policy is met for every PR could also help us determine a definitive owner or set of owners for every file.

We could probably make this work with some sort of post-processing but that might require reimplementing some of Policy Bot elsewhere, which we'd rather not do.

@bluekeyes
Copy link
Member

In some sense, Policy Bot already implements this behavior. If every rule in your policy has a file predicate (either changed_files or only_changed_files), when someone modifies a file that doesn't match any pattern, Policy Bot will post a failed status check. This is because Policy Bot requires every policy has at least one matching rule. When all rules have file patterns, files that don't match any patterns end up skipping all rules.

One limitation here is that this doesn't work if you're trying to combine the file-based rules with other rules (like having teams own files while giving one team permission to approve any change.)

If the existing skipped rule behavior doesn't meet your needs, could share some more details about how you envision this working? That would help me understand if there's a way to do it with existing features and if not, what a new feature might look like.

@robstolarz
Copy link
Author

I would really appreciate a way to tell Policy Bot about the break-glass rule (along the lines of what you mentioned about giving one team permission to approve any change) so that it doesn't prevent the existing file change behavior from working.

It's also worth noting that we had a rule like

approval_rules:
  - name: fallback
    requires:
      count: 0

to explicitly get around the ownership requirement behavior before. To that end, I wonder if it wouldn't make sense to make this behavior be explicitly configurable somehow, and give config authors the option as to how skipping should work ('disabled', 'enabled', 'only_rules_with_file_matches').

Alternatively, if we were given a new conjunction that allows us to determine whether some or all of the rules in the block were skipped (e.g. any_skipped:, all_skipped:), and we combined it with YAML anchors / aliases, that might provide ultimate explicit control over the file matching behavior. It might also be better aligned with the structure of the evaluation engine as it stands today.

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

No branches or pull requests

2 participants