Confusing behavior with skipped checks.

[Andrei-Predoiu]

I set up the following rule setup due to git API not reporting our "tests" statuses until they are finished.

The goal is to not require approval for dependabot if the test pass but require approvals from everyone else even if the tests pass. Instead, when someone other than dependabot makes a PR, the step is skipped and they can merge without approval.

What is the correct way to set my rule up?

policy:
  approval:
    - or:
      - and:
          - tests passed
          - owner has approved
      - and:
          - dependabot is making the PR
          - or:
              - tests passed
              - owner has approved
  disapproval:
    requires:
      organizations:
        - "org"
approval_rules:
  - name: tests passed
    if:
      has_successful_status:
        - "security checks"
        - "tests"
    requires:
      count: 0
  - name: owner has approved
    requires:
      count: 1
      teams:
        - "org/team"
  - name: dependabot is making the PR
    if:
      has_author_in:
        users:
          - "dependabot[bot]"
          - "dependabot-circleci[bot]"
    requires:
      count: 0

[bluekeyes]

The way we usually solve this is to directly make the tests status a required status check in GitHub. This way, the policy only reflects approval.

If you need to combine this all in the policy for some reason (maybe you want human developers to be able to ignore failing tests if they have approval), try combining the conditions in a single rule:

policy:
  approval:
    - or:
      - owner has approved
      - depandabot PR has passing tests
  disapproval:
    requires:
      organizations:
        - "org"

approval_rules:
  - name: owner has approved
    requires:
      count: 1
      teams:
        - "org/team"
  - name: dependabot PR has passing tests
    if:
      has_author_in:
        users:
          - "dependabot[bot]"
          - "dependabot-circleci[bot]"
      has_successful_status:
        - "security checks"
        - "tests"
    requires:
      count: 0

[misund]

In your suggested config, the dependabot PR has passing tests rule will be satisfied while the security checks and tests status checks are pending, even if they eventually will fail or time out. That is the confusing behavior.

The key name is has_successful_status, but the behavior is more like "does not have unsuccessful status".

[Andrei-Predoiu]

@bluekeyes In my opinion, sing policy bot is supposed to replace the need for other required status checks. This "workaround" of making tests a required status in GitHub doesn't even work if i want conditional approval based on author.

@misund Firstly not all CI's report statuses immediately, ours in some cases will not report while CI is queued and will not report some tests until they pass or fail. Secondly, this makes using policy-bot to set org wide standards impossible, our policy of "you must have this successful status" to be able to merge anything can just be bypassed by not reporting the status at all, fx deleting it from your CI workflow.

Another example where this behavior does not make sense (to me):

policy:
  approval:
    - or:
        - and:
            - tests passed
            - owner has approved
        - and:
            - dependabot is making the PR
            - or:
                - tests passed
                - owner has approved
  disapproval:
    requires:
      organizations:
        - "bestseller"
approval_rules:
  - name: tests passed
    if:
      has_successful_status:
        - "test"
        - "Security Checks"
    requires:
      count: 0
  - name: owner has approved
    requires:
      count: 1
      teams:
        - "....."
  - name: dependabot is making the PR
    if:
      has_author_in:
        users:
          - "dependabot[bot]"
          - "dependabot-circleci[bot]"
    requires:
      count: 0

In this case, the behavior we want is that if the PR is made by dependabot and the tests pass then it should not require approval and bulldozer can merge it. In reality, the has_author_in gets skipped if false and anyone can merge the PR without any approval, if the tests pass

[asvoboda]

I think the following statement is probably where the confusion lies:

In my opinion, using policy bot is supposed to replace the need for other required status checks

When using the bot internally, we don't hold this statement as true. Policy-bot is a gate for more complex review policies, but is not the only or single status check that should be made required.

For example, we mark policy-bot, CI, and some other internal only checks as the required status checks for repo branch protection. Each repo can configure additional checks beyond just policy-bot and CI. Once all the required status checks are passing, bulldozer will merge in PRs.

This lets us decouple trying to put everything into a policy and affords a bit more flexibility in maintaining a small set of policies that can work in almost all repos. That being said, I think there is maybe some merit in adding some new functionality to policy bot around status checks but I think it goes against how we were originally holding the problem. Let us think a bit internally on a reasonable path forward.

[bluekeyes]

I'm going to close this now that #752 is implemented and released in version 1.35.0. While the behavior of skipped checks may still be confusing, I tried to improve the documentation here and I believe there is now an alternative way (required conditions) to implement the desired behavior.

If that's not true, we can reopen this and discuss what is still missing.