Condition for not having specific label(s)

[GabrielCFigueira]

At my company, the need arose for a condition that returns true if the PR does not have a (specific) label. For this, it would be useful to have a "has_not_labels" condition, similar to "has_labels", that holds a list of labels and is satisfied if the PR does not have any of the labels in the list (otherwise, it is not satisfied).

I want to have a policy that does certain checks if a label is present, but just blankly approve if the label is not present.

Would this change make sense, and would you be open to a contributing PR?

[bluekeyes]

I want to have a policy that does certain checks if a label is present, but just blankly approve if the label is not present.

You can usually achieve policies like this with the existing conditions by combining general and more specific rules in an and clause. For example:

policy:
  approval:
    # note: the top-level condition of an approval policy is 'and'
    - labels require extra approval
    - all changes are approved

approval_rules:
  - name: labels require extra approval
    if:
      has_labels:
        - major-change
    requires:
      count: 2
      teams: ["my-org/major-change-approvers"]

  - name: all changes are approved
    requires:
      count: 0

With this policy:

• A PR with the major-change label matches the first rule and requires approval from the major-change-approvers team. It also matches the second rule, but because of the implicit and condition, both rules must pass before the overall policy passes.
• A PR without the major-change label causes the first rule to evaluate as skipped and only matches the second rule. Skipped rules do not contribute to the overall result, so only the second rule counts and the overall policy passes.

Do you think something like this will work for you? If not, could you please provide some more details about a situation where the proposed has_not_labels condition is necessary?

[GabrielCFigueira]

Hey @bluekeyes, our use case is the following. We want PRs with a label to skip review or other status checks and be merged automatically. For that, we use policy-bot in tandem with bulldozer. For policy-bot we have this example policy (but we could need more than one emergency change rule):

policy:
   approval:
   - or:
     - emergency change
     - normal change

approval_rules:
- name: emergency change
   if:
     has_labels:
     - "emergency change"
     only_changed_files:
       paths:
       - "path/to/files/*.yml"
- name: normal change
   if:
     has_not_labels:
     - "emergency change"

And bulldozer:

version: 1

merge:
  method: merge
  trigger:
    labels: ["emergency change"]
  required_statuses:
  - "policy-bot: master"

With these policies, if a PR is created:

1. with the label "emergency change" and the changed files match "path/to/files/*.yml", the PR is merged by bulldozer
2. with the label "emergency change" and the changed files do not match "path/to/files/*.yml", the policy-bot status fails and bulldozer does not merge
3. without the label, the policy-bot status succeeds but bulldozer does not merge

With your suggested policy, situation 2 would fail, because the first rule would be skipped and the second would succeed, right? The problem here is that we need policy-bot status to fail in certain situations. Is there a way to achieve this with the existing conditions?

[bluekeyes]

Policy Bot is really designed to result in a success or a pending status, with the assumption that every pull request is ultimately approved through some rule. The error status your proposal relies on was meant as an actual error, to indicate that the policy is insufficient for all of the changes in the repository.

Since it sounds like your goal is to make sure that the policy-bot status is not passing when an emergency change modifies non-emergency files, one way to achieve this is to have a rule that remains in the pending state:

policy:
  approval:
  - normal change
  - or:
    - emergency change modifies allowed files
    - emergency change is approved

approval_rules:
- name: normal change
  requires:
    count: 0

- name: emergency change modifies allowed files
  if:
    has_labels:
    - "emergency change"
    only_changed_files:
      paths:
      - "path/to/files/.*\\.yml"
  requires:
    count: 0

- name: emergency change is approved
  if:
    has_labels:
    - "emergency change"
  requires:
    count: 1
    teams: ["org/emergency-change-approvers"]

To go through your three situations:

1. The normal change and emergency change modifies allowed files rules pass and the policy-bot status is green. The emergency change is approved rule is pending, but this does not change the overall status and Bulldozer can run.
2. The normal change rule passes, the emergency change modifies allowed files rule is skipped, and the emergency change is approved rule is pending. The overall status of the policy is pending, so Bulldozer does not run
3. The normal change rule passes and all other rules are skipped

This assumes that someone in an appropriate team can review an emergency change that modifies other files and then it would be safe to merge. If you really want to make sure that the rule can never pass, you can do something like requiring a very large number of reviews, multiple reviews from a single user, or a review from a user that doesn't exist. This is a bit of a hack, but means the rule can never move out of the pending state.

My general strategy for designing policies is to first pick some fallback rule that is acceptable for all possible changes. Usually this is approval by some trusted team or users. Then, add rules to the policy to modify the behavior from this state, either to add more approvals or to remove approvals in certain cases.

Hopefully that helps for your use case. If you still feel like a negative label condition is necessary, I'm open to reviewing a PR for a has_no_labels predicate or a new labels predicate with has_any, has_all, and has_none subkeys, kind of like the title and repository predicates.

[GabrielCFigueira]

So I tested out this configuration, and it seems to work without the negative check. However, it uncovered another problem (for which I can open another issue if you wish) that happens with this configuration and with the has_not_labels condition:

If the PR is created without the label at first, the policy-bot check will have the status "approved". However, if the label is added afterwards, there seems to be some race condition between bulldozer merging the PR (because it now has the label and the status check was approved) and bulldozer learning about the new status check of policy-bot. I had bulldozer merge PR's even though the reported status check was either "pending" or "failed". The bulldozer logs show it merging the PR and getting the new status check afterwards.

This can impact situation 2, where PRs that shouldn't be auto merged are merged.

[bluekeyes]

The race condition you describe is a limitation of how GitHub apps interact with GitHub. When you add a label to a PR, GitHub sends a webhook to all installed apps that subscribe to pull request events. There's no guarantee on the order in which these events are sent or the order in which the apps receive them. Policy Bot has to receive this event and process it before it can change the status back to pending. At the same time, Bulldozer is processing its own even and can see the PR state prior to the Policy Bot change.

I don't know of a way to fix this in the apps other than introducing arbitrary delays or making the bots aware of each other, both of which I'd prefer to avoid.

My recommendation to avoid this is to decouple the the approval conditions from the merge triggers. For our own projects, we use a merge when ready label that triggers Bulldozer, but does not change the approval conditions. I'm not sure if that would work for what you want to achieve, but I think finding a solution that does not rely on the order in which the bots evaluate events will be best.

[GabrielCFigueira]

Hey, thanks for the answer. I tried to think of a solution, but the race condition would always be present or the process would become too cumbersome. A label just for merging would still suffer the race condition if both labels were added at the same time I believe.

I ended up making bulldozer check policy-bot using the simulate api. That way I was able to get a guaranteed correct result. I extended the simulate api to support adding and ignoring labels in the request. This way, even if policy-bot hasn't received the github event, it will return the correct result (for merges based on labels).

I can open a pr for simulate api changes as well as the has_no_labels condition if you wish.