Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

Possible for submit queue to merge a sha other than what was tested #422

Closed
pmorie opened this issue Jan 28, 2016 · 6 comments
Closed

Possible for submit queue to merge a sha other than what was tested #422

pmorie opened this issue Jan 28, 2016 · 6 comments
Labels
area/mungegithub

Comments

@pmorie
Copy link
Contributor

pmorie commented Jan 28, 2016

There's a race condition in the submit queue where the queue may merge a sha other than what was tested:
For example: kubernetes/kubernetes#19716 : this PR was 5/5 checks green, then needed rebase after the merge bot had called for a final test. I rebased, repushed, and re-added the LGTM label, and the PR merged even though 1/5 checks was green (presumably CLA check) in github for that sha.

@eparis
@pmorie pmorie mentioned this issue Jan 28, 2016
Closed
@pmorie
Copy link
Contributor Author

pmorie commented Feb 3, 2016

@eparis does #450 fix this?

@ixdy
Copy link
Contributor

ixdy commented Feb 19, 2016

Possibly related race condition: I created kubernetes/kubernetes/pull/21519 with e2e-not-required. It was LGTM'd, and then I realized I needed to change something else. I force-pushed this change, but instead of removing the LGTM (since the sha1 changed) the submit queue merged the new commit.

This was referenced Apr 21, 2016
Merged
Closed
@eparis eparis mentioned this issue Jul 20, 2016
Closed
@euank
Copy link

euank commented Oct 17, 2016 •
edited
Loading

Still not fixed, I slipped something through after the lgtm label was removed: kubernetes/kubernetes#34958 (comment)

@ixdy
Copy link
Contributor

ixdy commented Nov 8, 2016

this happened again in kubernetes/kubernetes#36407.

@apelisse
Copy link
Contributor

apelisse commented Nov 8, 2016

This one is probably even worse. It has do-not-merge and doesn't have lgtm...

@ixdy
Copy link
Contributor

ixdy commented Nov 8, 2016

Well, it's basically the same issue, I think. It had lgtm when SQ started testing it, and then I removed lgtm and added do-not-merge, but the SQ never looked again, apparently.

apelisse pushed a commit to apelisse/contrib that referenced this issue Nov 8, 2016
mungegithub: Fixes kubernetes-retired#422
0e73278 
At least attempts to fix one or two of the problems in kubernetes-retired#422.

Refresh the pull-request after we've done the tests to:
- Make sure it is still mergeable (doesn't have do-no-merge label or
still has lgtm label)
- The SHA hasn't changed, so that we don't commit something we haven't
tested
@apelisse apelisse mentioned this issue Nov 8, 2016
Merged
k8s-github-robot pushed a commit that referenced this issue Nov 8, 2016
Merge pull request #2008 from apelisse/mungebot-merge-race-condition
f071e3e 
Automatic merge from submit-queue

mungegithub: Fixes #422

At least attempts to fix one or two of the problems in #422.

Refresh the pull-request after we've done the tests to:
- Make sure it is still mergeable (doesn't have do-no-merge label or
still has lgtm label)
- The SHA hasn't changed, so that we don't commit something we haven't
tested
foxish pushed a commit to foxish/contrib that referenced this issue Jan 20, 2017
mungegithub: Fixes kubernetes-retired#422
f77b52b 
At least attempts to fix one or two of the problems in kubernetes-retired#422.

Refresh the pull-request after we've done the tests to:
- Make sure it is still mergeable (doesn't have do-no-merge label or
still has lgtm label)
- The SHA hasn't changed, so that we don't commit something we haven't
tested
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
area/mungegithub
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@pmorie @apelisse @euank @ixdy @eparis