Support for multiple partial approvals joining together

[dsw2127]

In repo we often have different sets of files that can be approved by different groups of people. Often a PR will include changes to multiple files that are owned by different groups. If we have several rules OR'd together with if_only_changed_files, then only PRs that touch files owned by a single group can be approved that way. Instead of all-or-nothing approval rules, we would like to have an approval be accepted for a subset of files in a PR, so it can be joined together with other approvals.

For example if group A owns files 1 and 2, group B owns files 3 and 4, and all other files, including new files (thus dynamic) require approval by an admin group C, it would be great if we could design a policy such that if someone submitted a PR touching files 2 and 4 policy bot would pass if someone from group A (covering file 2) and someone from group B (covering file 4) approve (but not if only one of them do). For something simple like this example one could maybe use an AND of changed_files rules, but it would be hard to cover the group C case without explicitly listing all the files in a negative lookahead (if that even works), which gets crazy if there are many files and many groups

[bluekeyes]

In the case you describe, can the admin group C always approve all changes? Or are there certain changes that require approval from groups A/B in addition to the admins in group C?

If the former, I think you can do something like this:

policy:
  approval:
    - or:
      # no file conditions for admin approval
      - admins approved

      # each group rule uses changed_files for its paths
      - and:
        - group A approved any group A changes
        - group B approved any group B changes
        # ...

If the non-admin groups must always approve changes to their files, you can do something like this:

policy:
  approval:
    # each group rule uses changed_files for its paths
    - group A approved any group A changes
    - group B approved any group B changes
    # ...
    - or:
      # no file conditions for admin approval
      - admins approved
      # uses only_changed_files with ALL of the files from the group rules above
      - only group files were changed

That doesn't require negative lookahead (which isn't supported), but does require listing out all of the files for each non-admin group a second time. But if you're generating the policy based on the groups to begin with, maybe adding the same paths in a second place isn't that bad.

[dsw2127]

In my particular case the admin group C can always approve. That said, your first example wouldn't work because if a PR was submitted that for example touched a group A file and a "restricted" file, the "and" block would pass and thus the overall policy would pass, which is not what we want (we should require group C approval for a PR like that).

I ended up doing something like:

policy:
  approval:
    - or:
      # Group files block - iff all changes to any group files have been approved by the relevant groups,
      # and only group files have been modified, this block will pass
      - and:
        - changes to A files have been approved by group A
        - changes to B files have been approved by group B
        - or:
          # If non-group files have been modified, the below rule will be skipped, so we need
          # to have the admin rules to make sure changes to other files are sufficiently approved
          - if only group files have been modified allow
          - admin team has approved

      # The below rules are duplicated to allow approval even if not all relevant groups have
      # approved their owned files, for emergency or broad changes
      - admin team has approved

This works for this case, especially as we are programmatically generating the policy file. However, since all of the files need to be added both as rules allowing their group to approve them, AND added to the "if only group files have been modified allow" rule, it would be easy to make a mistake.