tide not honoring multiple reviewer branch protection

[petr-muller]

Migrated from kubernetes/test-infra#23031 reported by @dhaiducek :

What happened:

• With this organization-level tide configuration:

tide:
  queries:
  - labels:
    - approved
    - lgtm
    missingLabels:
    - 'dco-signoff: no'
    - do-not-merge/hold
    - do-not-merge/invalid-owners-file
    - do-not-merge/work-in-progress
    - needs-rebase
   orgs:
   - <org>

• Set reviewers in branch-protection for a particular repo:

branch-protection:
  orgs:
    <org>:
      repos:
        <repo>:
          protect: true
          required_pull_request_reviews:
            dismiss_stale_reviews: true
            required_approving_review_count: 2

• tide merges PRs without the second review (or any review, really--it'll merge with /lgtm without an actual review)

What you expected to happen:

• With branch protection rules in place, I'd expect tide to respect the reviewer count and not merge PRs until then

How to reproduce it (as minimally and precisely as possible): (configuration above)
Please provide links to example occurrences, if any:
Anything else we need to know?:

• I found a configuration for tide queries, and was wondering what this was and whether this was the missing piece for honoring reviewers or if multiple reviewers was not currently a feature of tide:

tide:
  queries:
    reviewApprovedRequired: true

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[kaovilai]

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues.

This bot triages un-triaged issues according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue as fresh with /remove-lifecycle stale
• Close this issue with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[petr-muller]

/remove-lifecycle stale
/kind bug

Looking at this, it is actually quite weird: required_approving_review_count: 2 is a GH branch protection concept, this is something GitHub should enforce at merges, not Tide. I'd expect Tide to maybe try and fail similarly to #269 , but not succeed. Which means that maybe the GH branch protection is not properly synced by branchprotector? Not sure.

[dhaiducek]

Interesting. Some more data points: I double checked our repo. The setting does indeed appear to be in place, and I can't manually click the "merge" button in GitHub, even with one approving review:

So it's strange that Tide would be able to merge it after one review. The bot is an admin (which I think might be a requirement?), so it seems it might not be considering the reviewer setting as it does the passing/failed statuses. Though, if I recall correctly, Tide does block if the lgtm/approved labels are present but there's no GitHub approval.

[kaovilai]

The github UI do block merge. Merge by tide however isn't blocked with less than required reviews.

[kaovilai]

If tide merge with git merge command behind the scenes then github web ui would display PR as merged.

If we can check if it hits some web API rather than git commands that might help.

[petr-muller]

The bot is an admin (which I think might be a requirement?)

This is it - reading about protected branches it says

By default, the restrictions of a branch protection rule don't apply to people with admin permissions to the repository or custom roles with the "bypass branch protections" permission. You can optionally apply the restrictions to administrators and roles with the "bypass branch protections" permission, too. For more information, see "Managing custom repository roles for an organization".

Which means Tide as admin can merge PRs over branch protection unless the strict Rules applied to everyone including administrators config is enabled for the branch protection rule - I think this would give you #269 behavior.

Making Tide playing nicer with Github branch protection would be really nice, considering how GH features evolved over time. Right now I think we are hitting Tide model limits - it evaluates mergeability using mostly GH search (label presence/absence) and job results, and relies on lgtm / approve labels for reviews - and the appropriate plugins do not take GH's evolving logic into consideration.

🤔

[kaovilai]

We did try making tide not admin, does not work.