Skip to content
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

PR with "Change requested" leads to Tide repeatedly attempting MERGE #269

Open
sdowell opened this issue Sep 5, 2024 · 1 comment
Open
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@sdowell
Copy link

sdowell commented Sep 5, 2024

What happened:

For a Github PR that has passed all checks but has the Change requested state, Tide repeatedly attempts to MERGE but fails (presumably rejected by Github due to the Change requested state).

What you expected to happen:

I would expect Prow/Tide to be aware of the Change requested state and that it is blocking - and therefore not try to perform MERGE.

How to reproduce it (as minimally and precisely as possible):

  • Create a pull request
  • Request changes on the PR
  • Pass all other checks (e.g. presubmits, lgtm, approve, etc)
  • Observe that Tide repeatedly attempts to perform MERGE and fails

Please provide links to example occurrences, if any:

Here is an example PR and tide history:

Anything else we need to know?:
N/A


Copied over from kubernetes/test-infra#27750

@petr-muller petr-muller added the kind/bug Categorizes issue or PR as related to a bug. label Sep 20, 2024
@petr-muller
Copy link
Contributor

Copying some relevant information here from a previous attempt to fix this via approve plugin:

Many advanced branch protection features enforcing PR merge eligibility are younger than Tide and the big Prow orgs (kube, openshift) are mostly not using them. Generally Tide expects a PR to be mergeable once it passes Tide's criteria (query match + required jobs pass) and considers inability to merge to be "user errors".

I think that in today's world this does not stand. I'd prefer the solution to be

  1. Tide to properly surface any occurrences when it thinks it may merge but is disallowed to do so
  2. Tide to check as many mergeability properties as possible in its isAllowedToMerge method to avoid thinking it may merge a PR when GH would disallow it

It is not entirely straightforward. 1) is a problem because the status context (which right now is the only PR-facing Tide user interface, maybe with the exception of Tide PR dashboard which people don't really use that much) is controlled by a separate controller than the one that actually tries to merge. 2) is also not easy because checking mergeability for BP would require Tide asking about more properties of the PR (and possibly the repo, to check BP requirements).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants