Skip to content

Commit 9c489a3

Browse files
cjllanwarnecjllanwarne
authored
[ci] Add PR state reporting (#15307)
## Change Description - Fixes a bug that stops some PR statuses being set to Pass, even though the build batch succeeded - Add some simple elements to help debug "stuck" PRs and answer "why isn't it re-running / merging already" ## Security Assessment - This change potentially impacts the Hail Batch instance as deployed by Broad Institute in GCP ### Impact Rating - This change has a low security impact ### Impact Description Normal caution around PR titles being injected into html pages. But no different than existing elements, and we use jinja2 formatting via aiohttp which auto-escapes the titles. Otherwise, just a read-only change, other than setting PR statuses. But note that PR status determination happens separately from the internal mergeability calculation, so even if that's wrong, the PR merge is still gated by the same logic ### Appsec Review - [ ] Required: The impact has been assessed and approved by appsec
1 parent 8c8570c commit 9c489a3

3 files changed

Lines changed: 110 additions & 12 deletions

File tree

‎ci/ci/ci.py‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,34 @@ async def _populate_active_pr_context(page_context: Dict[str, Any], wb: WatchedB
197197
pr = wb.prs[pr_number]
198198
page_context['pr'] = pr
199199
page_context['active_pr'] = True
200+
201+
# Merge eligibility checklist
202+
page_context['review_approved'] = pr.review_state == 'approved'
203+
page_context['checks_all_pass'] = len(pr.last_known_github_status) > 0 and pr.build_succeeding_on_all_platforms()
204+
page_context['is_up_to_date'] = pr.is_up_to_date()
205+
page_context['blocking_labels'] = [label for label in pr.labels if label in ('WIP', 'stacked PR')]
206+
page_context['is_mergeable'] = pr.is_mergeable()
207+
208+
# Merge queue: approved, non-blocked, non-failing PRs with higher priority
209+
_do_not_merge = frozenset(('WIP', 'stacked PR'))
210+
page_context['build_failing'] = pr.build_failed_on_at_least_one_platform()
211+
page_context['prs_ahead_in_queue'] = [
212+
{'number': p.number, 'title': p.title, 'is_merge_candidate': p is wb.merge_candidate}
213+
for p in wb.prs_in_merge_priority_order()
214+
if p.number != pr.number
215+
and p.merge_priority() > pr.merge_priority()
216+
and p.review_state == 'approved'
217+
and not any(label in _do_not_merge for label in p.labels)
218+
and not p.build_failed_on_at_least_one_platform()
219+
]
220+
page_context['is_merge_candidate'] = wb.merge_candidate is not None and wb.merge_candidate.number == pr.number
221+
222+
# Deploy batch blocking next merge (running but not yet complete)
223+
deploy_batch = wb.deploy_batch
224+
page_context['blocking_deploy_batch_id'] = (
225+
deploy_batch.id if deploy_batch and isinstance(deploy_batch, Batch) and wb.deploy_state is None else None
226+
)
227+
200228
batch = pr.batch
201229
if batch:
202230
if isinstance(batch, Batch):

‎ci/ci/github.py‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -395,11 +395,7 @@ def config(self):
395395
def github_status_from_build_state(self) -> GithubStatus:
396396
if self.build_state in ('failure', 'error'):
397397
return GithubStatus.FAILURE
398-
if (
399-
self.build_state == 'success'
400-
and self.batch
401-
and self.batch.attributes['target_sha'] == self.target_branch.sha
402-
):
398+
if self.build_state == 'success' and self.batch:
403399
return GithubStatus.SUCCESS
404400
return GithubStatus.PENDING
405401

‎ci/ci/templates/pr.html‎

Lines changed: 81 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,17 +9,91 @@ <h1>{{ pr.title }} <a rel="noopener" href="https://github.com/{{ repo }}/pull/{{
99
<div class="attributes">
1010
<div>PR <a rel="noopener" href="https://github.com/{{ repo }}/pull/{{ pr.number }}">#{{ pr.number }}</a> status:
1111
{% if active_pr %}
12-
<span class="material-symbols-outlined" style="color: green; vertical-align: middle; font-size: 1rem; font-variation-settings: 'FILL' 1, 'wght' 700;">merge</span> open (live)
12+
<span class="material-symbols-outlined" style="color: green; vertical-align: middle; font-size: 1.3rem; font-variation-settings: 'FILL' 1, 'wght' 700;">merge</span> open (live)
1313
{% elif pr.merged %}
14-
<span class="material-symbols-outlined" style="color: purple; vertical-align: middle; font-size: 1rem; font-variation-settings: 'FILL' 1, 'wght' 700;">merge</span> merged
14+
<span class="material-symbols-outlined" style="color: purple; vertical-align: middle; font-size: 1.3rem; font-variation-settings: 'FILL' 1, 'wght' 700;">merge</span> merged
1515
{% elif pr.merged is none %}
16-
<span class="material-symbols-outlined" style="color: grey; vertical-align: middle; font-size: 1rem; font-variation-settings: 'FILL' 1, 'wght' 700;">question_mark</span> closed
16+
<span class="material-symbols-outlined" style="color: grey; vertical-align: middle; font-size: 1.3rem; font-variation-settings: 'FILL' 1, 'wght' 700;">question_mark</span> closed
1717
{% else %}
18-
<span class="material-symbols-outlined" style="color: red; vertical-align: middle; font-size: 1rem; font-variation-settings: 'FILL' 1, 'wght' 700;">block</span> closed without merging
18+
<span class="material-symbols-outlined" style="color: red; vertical-align: middle; font-size: 1.3rem; font-variation-settings: 'FILL' 1, 'wght' 700;">block</span> closed without merging
1919
{% endif %}
2020
</div>
2121
</div>
2222

23+
{% if active_pr %}
24+
<div class="attributes">
25+
<h2>Merge Eligibility</h2>
26+
<ul style="list-style: none; padding: 0; margin: 0;">
27+
<li>
28+
{% if review_approved %}
29+
<span class="material-symbols-outlined" style="color: green; vertical-align: middle; font-size: 1.3rem; font-variation-settings: 'FILL' 1, 'wght' 700;">check_circle</span> PR approved
30+
{% elif pr.review_state == 'changes_requested' %}
31+
<span class="material-symbols-outlined" style="color: red; vertical-align: middle; font-size: 1.3rem; font-variation-settings: 'FILL' 1, 'wght' 700;">block</span> Changes requested
32+
{% else %}
33+
<span class="material-symbols-outlined" style="color: grey; vertical-align: middle; font-size: 1.3rem; font-variation-settings: 'FILL' 1, 'wght' 700;">pending</span> Awaiting review
34+
{% endif %}
35+
</li>
36+
<li>
37+
{% if checks_all_pass %}
38+
<span class="material-symbols-outlined" style="color: green; vertical-align: middle; font-size: 1.3rem; font-variation-settings: 'FILL' 1, 'wght' 700;">check_circle</span> All checks passing
39+
{% elif pr.build_state is none %}
40+
<span class="material-symbols-outlined" style="color: grey; vertical-align: middle; font-size: 1.3rem; font-variation-settings: 'FILL' 1, 'wght' 700;">pending</span> Build in progress
41+
{% else %}
42+
<span class="material-symbols-outlined" style="color: red; vertical-align: middle; font-size: 1.3rem; font-variation-settings: 'FILL' 1, 'wght' 700;">cancel</span> Checks not passing ({{ pr.build_state }})
43+
{% endif %}
44+
</li>
45+
<li>
46+
{% if is_up_to_date %}
47+
<span class="material-symbols-outlined" style="color: green; vertical-align: middle; font-size: 1.3rem; font-variation-settings: 'FILL' 1, 'wght' 700;">check_circle</span> Target SHA matches HEAD
48+
{% elif is_merge_candidate %}
49+
<span class="material-symbols-outlined" style="color: orange; vertical-align: middle; font-size: 1.3rem; font-variation-settings: 'FILL' 1, 'wght' 700;">warning</span> Target SHA does not match HEAD - rebuild starting shortly
50+
{% else %}
51+
<span class="material-symbols-outlined" style="color: orange; vertical-align: middle; font-size: 1.3rem; font-variation-settings: 'FILL' 1, 'wght' 700;">warning</span> Target SHA does not match HEAD - will rebuild once merge candidate
52+
{% endif %}
53+
</li>
54+
{% if blocking_labels %}
55+
<li>
56+
<span class="material-symbols-outlined" style="color: red; vertical-align: middle; font-size: 1.3rem; font-variation-settings: 'FILL' 1, 'wght' 700;">cancel</span> Blocked by label: {{ blocking_labels | join(', ') }}
57+
</li>
58+
{% endif %}
59+
</ul>
60+
</div>
61+
62+
<div class="attributes">
63+
<h2>Merge Queue</h2>
64+
{% if build_failing or not review_approved %}
65+
<p>Not in the merge queue -
66+
{% if build_failing and not review_approved %}needs approval and build is failing.
67+
{% elif not review_approved %}needs approval.
68+
{% else %}build is failing.
69+
{% endif %}
70+
</p>
71+
{% else %}
72+
{% if prs_ahead_in_queue %}
73+
<p>{{ prs_ahead_in_queue | length }} PR(s) ahead in queue:</p>
74+
<ul>
75+
{% for ahead_pr in prs_ahead_in_queue %}
76+
<li><a href="{{ base_path }}/watched_branches/{{ wb.index }}/pr/{{ ahead_pr['number'] }}">#{{ ahead_pr['number'] }}</a> - {{ ahead_pr['title'] }}{% if ahead_pr['is_merge_candidate'] %} <em>(merge candidate)</em>{% endif %}</li>
77+
{% endfor %}
78+
</ul>
79+
{% elif is_mergeable %}
80+
<p>No approved PRs ahead - this PR is next to merge.</p>
81+
{% elif is_merge_candidate %}
82+
<p>This PR is the current merge candidate.</p>
83+
{% else %}
84+
<p>Not yet in the merge queue - awaiting build results.</p>
85+
{% endif %}
86+
{% if blocking_deploy_batch_id %}
87+
<p>
88+
<span class="material-symbols-outlined" style="color: orange; vertical-align: middle; font-size: 1.3rem; font-variation-settings: 'FILL' 1, 'wght' 700;">hourglass_top</span>
89+
Deploy batch <a rel="noopener" href="{{ batch_base_url }}/batches/{{ blocking_deploy_batch_id }}">{{ blocking_deploy_batch_id }}</a> is running - merge blocked until it completes.
90+
</p>
91+
{% endif %}
92+
{% endif %}
93+
</div>
94+
{% endif %}
95+
96+
<h2>Current Build</h2>
2397
{% if batch is defined %}
2498
<div class="attributes">
2599
<div>batch: <a rel="noopener" href="{{ batch_base_url }}/batches/{{ batch['id'] }}">{{ batch['id'] }}</a></div>
@@ -41,15 +115,15 @@ <h1>{{ pr.title }} <a rel="noopener" href="https://github.com/{{ repo }}/pull/{{
41115
{% endif %}
42116

43117
{% if logging_queries is not none %}
44-
<h2>Logging Queries</h2>
118+
<h3>Logging Queries</h3>
45119
<div class="logging-queries">
46120
{% for name, link in logging_queries.items() %}
47121
<div><a href="{{ link }}">{{ name }}</a></div>
48122
{% endfor %}
49123
</div>
50124
{% endif %}
51125

52-
<h2>Jobs</h2>
126+
<h3>Jobs</h3>
53127
{{ filtered_jobs(running, failed, pending, completed) }}
54128
{% elif exception is defined %}
55129
<p>Build error:</p>
@@ -58,7 +132,7 @@ <h2>Jobs</h2>
58132
</pre>
59133
{% else %}
60134
{% if active_pr %}
61-
<p>Build queued — waiting for a build slot. <a href="{{ ci_base_url }}">View CI dashboard</a>.</p>
135+
<p>Build queued - waiting for a build slot. <a href="{{ ci_base_url }}">View CI dashboard</a>.</p>
62136
{% else %}
63137
<p>No builds found.</p>
64138
{% endif %}

0 commit comments

Comments
 (0)