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.

Sign up for GitHub

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

Jump to bottom

CI considers PRs as merge candidates with only 1 review #14660

Closed
cjllanwarne opened this issue Aug 19, 2024 · 0 comments · Fixed by #14661
Closed

CI considers PRs as merge candidates with only 1 review #14660

cjllanwarne opened this issue Aug 19, 2024 · 0 comments · Fixed by #14661
Assignees
@cjllanwarne
Labels
bug infrastructure Non-user-facing change.

Comments

@cjllanwarne
Copy link
Collaborator

What happened?

CI has a concept of merge candidate to track which PRs are worth re-testing even if their original test set has passed, so that we are only merging PRs which have been tested against the head of main.

CI needs updating to reflect that we now expect 2 github approvals per review.

Version

CI (current)

Relevant log output

No response
@cjllanwarne cjllanwarne added needs-triage A brand new issue that needs triaging. infrastructure Non-user-facing change. bug and removed needs-triage A brand new issue that needs triaging. labels Aug 19, 2024
@cjllanwarne cjllanwarne self-assigned this Aug 19, 2024
This was referenced Aug 19, 2024
Closed
Merged
hail-ci-robot pushed a commit that referenced this issue Aug 25, 2024
@cjllanwarne
[CI] Use github's graphQL to query review state directly (#14661)
0b2087d 
Fixes #14660 by using the graphQL API to query github directly. Replaces
our current parallel interpretation of reviews into a review decision,
which is brittle if we ever change review requirements in github again.

Tested by manually updating the live CI to use the test batch generated
image. Results:
- Review decisions correctly fetched from github, not based on CI's
parallel interpretation of individual reviews:

![image](https://github.com/user-attachments/assets/67c03aa9-000a-44e7-91aa-3a42d04238dc)
- No merge candidate was being incorrectly nominated (in particular,
#14645 is now considered pending, rather than approved, which is what we
are currently, incorrectly, calculating)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cjllanwarne cjllanwarne

Labels
bug infrastructure Non-user-facing change.
Projects
None yet
Milestone
No milestone
1 participant
@cjllanwarne