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
Tracked by #290 ...
sdowell opened this issue Sep 5, 2024 · 2 comments
Open
Tracked by #290 ...
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.

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

@k8s-triage-robot
Copy link

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

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 19, 2024
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. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants