New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Confusing behavior with skipped checks. #627
Comments
The way we usually solve this is to directly make the 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 |
In your suggested config, the The key name is |
@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. 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. |
I think the following statement is probably where the confusion lies:
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. |
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. |
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?
The text was updated successfully, but these errors were encountered: