Skip to content

Commit d1da978

Browse files
committed
github_sync: smarter target branch selection
Implement "sticky" target branch: if a series has an associated open PR without merge-conflict, then do not consider other target branches. The config allows for multiple target branches to be associated with the same tag, for example: "tag_to_branch_mapping": { "tag": ["target1", "target2"] } Whenever a series is being synced, KPD tries to apply it to each target branch in order, stopping at the first successful apply. Then it opens/updates a tracking PR, and closes all other PRs for this series. This works fine most of the time. However, this logic may lead to series suddenly switching branches. Example scenario: * v1 successfully applied to target1, pr1 is created for it * submitter receives CI report with v1 applied to target1 * v2 fails to apply to target1, but succeeds on target2 * KPD opens pr2 for target2, and closes pr1 * submitter receives CI report with v2 applied to target2 with no indication that something happened What should've happened in this case is pr1 labeled as "merge-conflict" and target2 not tried. The "stickiness" is implemented by checking existing PRs when determining the list of target branches to apply the series to. Signed-off-by: Ihor Solodrai <[email protected]>
1 parent da8dd15 commit d1da978

File tree

2 files changed

+75
-3
lines changed

2 files changed

+75
-3
lines changed

kernel_patches_daemon/github_sync.py

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,34 @@ async def checkout_and_patch_safe(
187187
)
188188
return None
189189

190+
async def select_target_branches_for_subject(
191+
self, subject: Subject, tag_mapped_branches: List[str]
192+
) -> List[str]:
193+
if len(tag_mapped_branches) == 1:
194+
return tag_mapped_branches
195+
196+
def has_merge_conflict_label(pr: PullRequest) -> bool:
197+
for label in pr.get_labels():
198+
if label.name == "merge-conflict":
199+
return True
200+
return False
201+
202+
# Check if a single relevant open PR without merge conflicts exists.
203+
# If yes, then pick it without trying other target branches.
204+
subject_pr_targets = []
205+
for branch in tag_mapped_branches:
206+
worker = self.workers[branch]
207+
subj_branch = await worker.subject_to_branch(subject)
208+
for pr in worker.prs.values():
209+
if pr.head.ref == subj_branch and not has_merge_conflict_label(pr):
210+
subject_pr_targets.append(branch)
211+
212+
if len(subject_pr_targets) == 1:
213+
return [subject_pr_targets[0]]
214+
215+
# If no sticky target is determined, then return all branches
216+
return tag_mapped_branches
217+
190218
async def sync_relevant_subject(self, subject: Subject) -> None:
191219
"""
192220
1. Get Subject's latest series
@@ -206,8 +234,11 @@ async def sync_relevant_subject(self, subject: Subject) -> None:
206234
)
207235
return
208236

209-
last_branch = mapped_branches[-1]
210-
for branch in mapped_branches:
237+
target_branches = await self.select_target_branches_for_subject(
238+
subject, mapped_branches
239+
)
240+
last_branch = target_branches[-1]
241+
for branch in target_branches:
211242
worker = self.workers[branch]
212243
# PR branch name == sid of the first known series
213244
pr_branch_name = await worker.subject_to_branch(subject)

tests/test_github_sync.py

Lines changed: 42 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,11 @@
1717
from kernel_patches_daemon.branch_worker import NewPRWithNoChangeException
1818
from kernel_patches_daemon.config import KPDConfig
1919
from kernel_patches_daemon.github_sync import GithubSync, HEAD_BASE_SEPARATOR
20-
from tests.common.patchwork_mock import init_pw_responses, load_test_data, PatchworkMock
20+
from tests.common.patchwork_mock import (
21+
init_pw_responses,
22+
load_test_data,
23+
PatchworkMock,
24+
)
2125

2226
TEST_BRANCH = "test-branch"
2327
TEST_BPF_NEXT_BRANCH = "test-bpf-next"
@@ -278,6 +282,43 @@ async def test_sync_relevant_subject_success_second_branch(self) -> None:
278282
list(self._gh.workers.values()), pr_mock
279283
)
280284

285+
def _setup_test_select_target_branches_for_subject(self):
286+
series_prefix = "series/123123"
287+
subject_mock = MagicMock()
288+
subject_mock.subject = "Test subject"
289+
subject_mock.branch = AsyncMock(return_value=series_prefix)
290+
291+
pr_mock = MagicMock()
292+
pr_mock.head.ref = f"{series_prefix}{HEAD_BASE_SEPARATOR}{TEST_BPF_NEXT_BRANCH}"
293+
294+
worker_mock = self._gh.workers[TEST_BPF_NEXT_BRANCH]
295+
worker_mock.prs = {subject_mock.subject: pr_mock}
296+
297+
return subject_mock, pr_mock
298+
299+
async def test_select_target_branches_for_subject(self) -> None:
300+
subject_mock, _ = self._setup_test_select_target_branches_for_subject()
301+
mapped_branches = [TEST_BRANCH, TEST_BPF_NEXT_BRANCH]
302+
303+
selected_branches = await self._gh.select_target_branches_for_subject(
304+
subject_mock, mapped_branches
305+
)
306+
307+
self.assertEqual(selected_branches, [TEST_BPF_NEXT_BRANCH])
308+
309+
async def test_select_target_branches_for_subject_merge_conflict(self) -> None:
310+
subject_mock, pr_mock = self._setup_test_select_target_branches_for_subject()
311+
label = MagicMock()
312+
label.name = "merge-conflict"
313+
pr_mock.get_labels = MagicMock(return_value=[label])
314+
mapped_branches = [TEST_BRANCH, TEST_BPF_NEXT_BRANCH]
315+
316+
selected_branches = await self._gh.select_target_branches_for_subject(
317+
subject_mock, mapped_branches
318+
)
319+
320+
self.assertEqual(selected_branches, mapped_branches)
321+
281322
@aioresponses()
282323
async def test_sync_patches_pr_summary_success(self, m: aioresponses) -> None:
283324
"""

0 commit comments

Comments
 (0)