Confusing behavior with skipped checks. about policy-bot HOT 5 CLOSED

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".

from policy-bot.

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.

from policy-bot.

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

from policy-bot.

@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

from policy-bot.

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.

from policy-bot.