Feature Request: Predicate to skip rule if a file was changed

[erikburt]

Situation

We are running into limitations with policy bot and would like a new feature.

Given 2 PRs which should each trigger a rule specific to them

• PR 1 - trigger rule 1 and skip rule 2
  • a file is introduced: `foo/bar.json
  • an assortment of miscellaneous files are edited at the root or in many other directories
  • no files in foo/archive/ are touched
• PR 2 - trigger rule 2 and skip rule 1
  • the file is moved: foo/bar.json -> foo/archive/bar.json

In this exact scenario, there's no way simple way to create a rule which would apply two rules separately. This is because there's no way to negate/exclude certain paths explicitly. If go supported negative look-ahead regex statements, this would not be a problem.

Current Rule Predicates (Convoluted)

- name: Rule 1
    if:
      # The idea here is to include all possible file changes during PR 1.
      # There is no way to negate paths, so we have to create an exhaustive list.
      # Which will effectively allow us to negate paths, by excluding them from the list.
      # Purposefully Ignored Paths
      # - .github/
      # - foo/*/ (for the purpose of excluding directory/archive)
      # - node_modules/
      only_changed_files:
        paths:
          # json changes in root directory - ie. no / in the path/filename
          - "^[a-zA-Z0-9_.-]+json$"
          # specific dot files in root directory
          # changed files in directory/* but not in directory/*/* (ie. no subdirectories)
          - "^foo/[^/]+$"
          # changes in every other subdirectory (allows recursive subdirectories)
          - "^directory-1/.*"
          - "^directory-2/.*"
# ...

- name: Rule 2 
    if:
      changed_files:
        paths:
          - "^foo/archive/.*"
...

Proposed Feature

Have a excludePaths feature for the changed_files predicate.

Behaviour:

• changed_files is satisfied if:
  • any file in the pull request matches any regular expression in the "paths" list, and every file in the pull request does not match any regular expression in the excludePaths list
  • If the "ignore" list is present, files in the pull request matching these regular expressions are ignored by both the paths and excludePaths rules

Solution

Having a functionality similar to the proposed excludePaths in changed_files would solve the above scenario we are facing because we could negate files explicitly, rather than having to try and negate files by creating an exhaustive list with only_changed_files.

• Example solution with changed files and exclude paths: paths: "^foo/a-zA-Z0-9_.-+$" and excludePaths: "^foo/archive/.*

---

I already have some local changes which I believe implement this functionality. Figured I'd open an issue and see if there's any alternatives suggested for negating files ~~before I PR this~~.

[bluekeyes]

Thanks for the proposal. Could you share a bit more about the approval conditions for each of your rules and how they combine as part of the overall policy? What is the overall behavior you are trying to achieve?

I've found that you can sometimes avoid the need for negative/exclude conditions with the right combination of rules, but you need to know the high-level goal to design such a policy.

[erikburt]

We have a process that happens in two steps:

Step 1

A PR is created and it :

1. Adds some amount json files to a specific directory located at the root of the repository (foo/bar-0.json, ..., foo/bar-n.json)
   • Not in any subdirectories of foo/
2. Modifies some amount of other existing files in the repository

We'd like to apply a policy which requires an approval from some user in group A.

Step 2

A PR is created and it:

1. Moves the json files introduced in step 1 from foo/ directory into foo/archive/ directory
   • git mv foo/*.json foo/archive/

We'd like to apply a policy which requires an approval from some user in group B.

---

From what I can tell - there's no way to segregate these rules in a simple way. I've explained the only solution I've been able to come up with (above). Unfortunately it's convoluted and requires updating if there's new directories which could be modified as part of step 1.

We'd like to apply these rules solely based off file changes, as most other things with the PRs are far too dynamic (naming, author, line count) etc.

If you have another way which accomplishes this, then I'd be glad to use that, although the exclusion logic makes this case very easy to solve.

[erikburt]

Another solution which doesn't overload changed_files predicate would be to add a couple more predicates like added_files modified_files deleted_files, also implementing similar functionality to changed_paths but limiting them to specific operations on those files.

ie. In Step 1 (above) we could limit the filtering to added_files: paths: - "^foo/a-zA-Z0-9_.-+json$", which wouldn't clash with the rename in step 2 which effectively deletes those files.

[bluekeyes]

I thought about this a bit and have a suggestion using existing features that may work:

policy:
  approval:
    - group B approved archive files
    - or:
        - group A approved foo files
        - pull request only archives files

approval_rules:
  - name: group B approved archive files
    if:
      changed_files:
        paths:
          - '^foo/archive/.*$'
    requires:
      count: 1
      team: ["org/group-b"]

  - name: group A approved foo files
    if:
      changed_files:
        paths:
          - '^foo/[^/]+\.json$'
    requires:
      count: 1
      team: ["org/group-a"]

  - name: pull request only archives files
    if:
      only_changed_files:
        paths:
          - '^foo/[^/]+\.json$'
          - '^foo/archive/.*$'
    requires:
      conditions:
        changed_files:
          paths:
          - '^foo/archive/.*$'

The idea is that groups A and B have to approve any PRs that change the paths they care about (the rules are skipped if the PRs only change other files.) Then, the pull request only archives files rule bypasses group A's approval in the situation where only group B needs to approve (moving files to the archive directory.) The conditions block makes sure that the bypass doesn't happen on PRs that only modify foo/*.json files, without also touching the archive directory.

This uses the new required conditions feature from #752, which is in the snapshot image, but not in a release yet. It also needs to be combined with additional rules to handle PRs that don't modify any of these special files.

I'll also admit it's non-obvious and that a new predicate would probably make it easier to write policies like this.

---

If that doesn't work and you do want to add a new predicate, I think either of:

• changed_no_files (or not_changed_files)
• added_files / modified_files / deleted_files

will work better than adding exclude_paths to the changed_files predicate. I think the exclude_paths sub-option will make the existing predicate hard to reason about.

Of those, my guess is that changed_no_files is more broadly useful than the modification type predicates.

[erikburt]

Thank you! I will try this out and get back to you 😄

[erikburt]

I appreciate the help with the rules you've setup, they accomplish the goals, thank you for taking the time with that!

As you've called out - it's non-obvious, and we'd like the rules to be simpler so they can be more easily maintained by other teams. So I've opened a new PR #756, implementing no_changed_files as suggested.