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

Tide “jamming” on unmergable PRs in repo with no prow presubmits. #6579

Closed
heckj opened this issue Feb 1, 2018 · 2 comments · Fixed by #6581
Closed

Tide “jamming” on unmergable PRs in repo with no prow presubmits. #6579

heckj opened this issue Feb 1, 2018 · 2 comments · Fixed by #6581
Assignees
Labels
area/prow Issues or PRs related to prow

Comments

@heckj
Copy link

heckj commented Feb 1, 2018

Recent large changes to kubernetes/website made a few PR’s click over to have conflicts on their merge, so PRs that were otherwise approved were unable to merge cleanly. One of these was kubernetes/website#7128, another kubernetes/website#6955 If the examples help.

I manually added a /hold since prow appeared to be hung for any other merges on the repo while these two reported they couldn’t merge cleanly. I’m not certain the the approve plugin verifies that the PR will merge cleanly as a separate check vs. the various tests that are included for the PR otherwise.

I wasn’t sure if this was related to #5541 or not, so mentioning it here.

@BenTheElder
Copy link
Member

AFAIK Normally merge-ability is confirmed by the tests jobs which merge code into master at the beginning of the job -- the PR can definitely still be approved while failing tests by design, sometimes tests are just flaking etc.

No other PRs merging while one of these approved is a bug though, tide should fall back to trying to merge single PRs instead of batches of PRs when this happens and wind up merging the other PRs.

/area prow
/assign @cjwagner

@k8s-ci-robot k8s-ci-robot added the area/prow Issues or PRs related to prow label Feb 1, 2018
@cjwagner
Copy link
Member

cjwagner commented Feb 1, 2018

I don't see any problems with the approve plugin. That is just responsible for the "[APPROVALNOTIFIER]" comment and controlling the approved label.

There definitely is a bug with Tide here though. Tide doesn't actually check if a PR is mergeable. It instead assumes that if all tests are passing against the latest master branch commit then the PR must be mergeable. However, none of the status context on k/website are actually run by prow so Tide cannot know what master commit the tests ran against. For these kinds of status contexts, Tide can only be sure that they are statuses for the most recent PR branch commit so it assumes they were run against the latest master branch commit if that is necessary.

I can fix this by fixing #4752 which should be real easy.

@cjwagner cjwagner changed the title Approve prow plugin “jamming” on unmergable PRs Tide “jamming” on unmergable PRs in repo with no prow presubmits. Feb 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/prow Issues or PRs related to prow
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants