logical operator or doesn't seem to work?

[DavidHe1127]

My requirement is as follow:

• Condition A - If a particular file is changed in a PR then the PR can only be merged if someone from team A approves it.
  In the meantime
• Condition B - Changes to any other files require 1 approval from team B.

The example setup kind of does what I want, so I made a few changes based on that for my scenario where only a particular file is changed.

However, based on the testing result, it seems that as long as Condition B is met (even if the approver isn't from team A), the PR is allowed to merge completely ignoring Condition A. This behaviour sounds about right to me given the nature of OR. However, according to what's said in the example config, my PR shouldn't be allowed for merging.

Also, I am not sure if this is by design that if no approval/disapproval rules are matched, check is failing as it requires at least one matched rule. However, shouldn't the rules/config from branch protection setup be used then? Or I completely misunderstand how policy bot works Or I have to always put a fallback rule to catch fallback cases?

My test is running on image tagged snapshot

[asvoboda]

Hey @DavidHe1127,

Could you post the policy you've created to help in debugging?

[DavidHe1127]

Expected:

if changes in src/a.ts, then need 1 approval from someone from YYY before PR can be merged

Actual:

As long as one approval received from XXX, PR is allowed to merge that totally ignoring rule Require 1 approval from YYY for changes made in src/a.ts

policy:
  approval:
    - or:
      - 1 approval from a member from XXX
      - Require 1 approval from YYY for changes made in src/a.ts

approval_rules:
  - name: 1 approval from a member from XXX
    requires:
      count: 1
      teams:
        - 'ORG/XXX'

  - name: Require 1 approval from YYY for changes made in src/a.ts
    if:
      only_changed_files:
        paths:
          - "src/a.ts"
    requires:
      count: 1
      teams:
        - 'ORG/YYY'

[asvoboda]

If you want to always require review if anyone changes src/a.ts, then you probably want to change two things.

1. The operator on L3 should be and instead of or.
2. You may want to change only_changed_files to changed_files

The example config that you linked is intended to show how changes can be auto approved if a user only makes a change to staging files, or if anyone on the "devtools" team approves.

This may differ for your use case a little bit, where instead you may be looking to always require review from a specific team if a file has changed. Let me know if my understanding of your problem is not correct.

[DavidHe1127]

@asvoboda Thanks for your suggestions. Will definitely give it a go.

[DavidHe1127]

@asvoboda Unfortunately, it didn't work as expected since it now asks for another tick from XXX since it's and now.

Just want to clarify what I want:

• if there is change and it is the only change made in src/a.ts, one approval from YYY is more than enough to merge the PR
• Otherwise, if changes are not made in src/a.ts, any approvals from XXX should be enough to make PR mergeable.

[bluekeyes]

This is one of the trickier situations to encode in Policy Bot and requires some path duplication, but I believe the following will do what you want:

policy:
  approval:
    - Team YYY approved changes to src/a.ts
    - or:
      - Team XXX approved
      - Only restricted files were changed

approval_rules:
  - name: Team YYY approved changes to src/a.ts
    if:
      changed_files:
        paths:
          - "^src/a.ts$"
    requires:
      count: 1
      teams:
        - 'ORG/YYY'
  
  - name: Team XXX approved
    requires:
      count: 1
      teams:
        - 'ORG/XXX'

  - name: Only restricted files were changed
    if:
      only_changed_files:
        paths:
          - "^src/a.ts$"
    requires:
      count: 0

Explanation:

1. The Team YYY rule uses changed_files so that their approval is always required when src/a.ts changes, even if the PR also changes other files
2. Because this is part of the implicit top-level and, it is always required. If src/a.ts is not changed, the rule is skipped and only the or sub-condition applies
3. The Team XXX rule is the catch-all approval for other files, so it has no file conditions
4. The Only restricted files rule bypasses the Team XXX rule when the PR only modifies files that are covered by one of the top-level "required" rules.

The end result should be that:

• If src/a.ts is modified, only YYY needs to approve
• If src/b.ts is modified, only XXX needs to approve
• If both src/a.ts and src/b.ts are modified, both YYY and XXX need to approve

[DavidHe1127]

Thanks all for help.