policy-bot icon indicating copy to clipboard operation
policy-bot copied to clipboard

Option to require all changed files be evaluated by a rule

Open robstolarz opened this issue 1 year ago • 2 comments

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.

robstolarz avatar Mar 09 '23 19:03 robstolarz

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.

bluekeyes avatar Mar 10 '23 01:03 bluekeyes

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.

robstolarz avatar Mar 20 '23 22:03 robstolarz