branch_worker: refactor PR ref parsing

A couple of helper functions refered to "series/123" part of a branch
name ("series/123=>target") as "base" which is quite confusing.

Refactor helpers that were parsing PR refs and their usage.
Move separator constants used for branch names to config.py

Signed-off-by: Ihor Solodrai <[email protected]>

Author: theihor

kernel_patches_daemon/branch_worker.py

@@ -37,7 +37,11 @@
 from github.Repository import Repository
 from github.WorkflowJob import WorkflowJob
 
-from kernel_patches_daemon.config import EmailConfig
+from kernel_patches_daemon.config import (
+    SERIES_ID_SEPARATOR,
+    SERIES_TARGET_SEPARATOR,
+    EmailConfig,
+)
 from kernel_patches_daemon.github_connector import GithubConnector
 from kernel_patches_daemon.github_logs import GithubFailedJobLog, GithubLogExtractor
 from kernel_patches_daemon.patchwork import Patchwork, Series, Subject
@@ -79,7 +83,7 @@
 ALREADY_MERGED_LOOKBACK = 100
 BRANCH_TTL = 172800  # 1 week
 PULL_REQUEST_TTL = timedelta(days=7)
-HEAD_BASE_SEPARATOR = "=>"
+
 KNOWN_OK_COMMENT_EXCEPTIONS = {
     "Commenting is disabled on issues with more than 2500 comments"
 }
@@ -398,21 +402,36 @@ def create_color_labels(labels_cfg: Dict[str, str], repo: Repository) -> None:
             repo.create_label(name=label, color=color)
 
 
-def get_base_branch_from_ref(ref: str) -> str:
-    return ref.split(HEAD_BASE_SEPARATOR)[0]
+def parse_pr_ref(ref: str) -> Dict[str, Any]:
+    # "series/123456=>target-branch" ->
+    # {
+    #   "series": "series/123456",
+    #   "series_id": 123456,
+    #   "target": "target-branch",
+    #  }
+    res = {}
+    tmp = ref.split(SERIES_TARGET_SEPARATOR, maxsplit=1)
+    res["series"] = tmp[0]
+    if len(tmp) >= 2:
+        res["target"] = tmp[1]
 
+    tmp = res["series"].split("/", maxsplit=1)
+    if len(tmp) >= 2:
+        res["series_id"] = int(tmp[1])
 
-def has_same_base_different_remote(ref: str, other_ref: str) -> bool:
-    if ref == other_ref:
-        return False
+    return res
 
-    base = get_base_branch_from_ref(ref)
-    other_base = get_base_branch_from_ref(other_ref)
 
-    if base != other_base:
-        return False
+def parsed_pr_ref_ok(parsed_ref: Dict[str, Any]) -> bool:
+    return "target" in parsed_ref and "series_id" in parsed_ref
 
-    return True
+
+def same_series_different_target(ref: str, other_ref: str) -> bool:
+    if ref == other_ref:
+        return False
+    ref1 = parse_pr_ref(ref)
+    ref2 = parse_pr_ref(other_ref)
+    return ref1["series"] == ref2["series"] and ref1["target"] != ref2["target"]
 
 
 def _reset_repo(repo, branch: str) -> None:
@@ -1081,7 +1100,7 @@ def filter_closed_pr(self, head: str) -> Optional[PullRequest]:
         return res
 
     async def subject_to_branch(self, subject: Subject) -> str:
-        return f"{await subject.branch}{HEAD_BASE_SEPARATOR}{self.repo_branch}"
+        return f"{await subject.branch}{SERIES_TARGET_SEPARATOR}{self.repo_branch}"
 
     async def sync_checks(self, pr: PullRequest, series: Series) -> None:
         # Make sure that we are working with up-to-date data (as opposed to
@@ -1224,9 +1243,10 @@ def expire_branches(self) -> None:
                 # that are not belong to any known open prs
                 continue
 
-            if HEAD_BASE_SEPARATOR in branch:
-                split = branch.split(HEAD_BASE_SEPARATOR)
-                if len(split) > 1 and split[1] == self.repo_branch:
+            parsed_ref = parse_pr_ref(branch)
+
+            if parsed_pr_ref_ok(parsed_ref):
+                if parsed_ref["target"] == self.repo_branch:
                     # which have our repo_branch as target
                     # that doesn't have any closed PRs
                     # with last update within defined TTL

kernel_patches_daemon/config.py

@@ -17,6 +17,9 @@
 
 logger = logging.getLogger(__name__)
 
+SERIES_TARGET_SEPARATOR = "=>"
+SERIES_ID_SEPARATOR = "/"
+
 
 class UnsupportedConfigVersion(ValueError):
     def __init__(self, version: int) -> None:

kernel_patches_daemon/github_sync.py

@@ -15,12 +15,16 @@
 from github.PullRequest import PullRequest
 from kernel_patches_daemon.branch_worker import (
     BranchWorker,
-    get_base_branch_from_ref,
-    has_same_base_different_remote,
-    HEAD_BASE_SEPARATOR,
+    parsed_pr_ref_ok,
+    same_series_different_target,
+    parse_pr_ref,
     NewPRWithNoChangeException,
 )
-from kernel_patches_daemon.config import BranchConfig, KPDConfig
+from kernel_patches_daemon.config import (
+    SERIES_TARGET_SEPARATOR,
+    BranchConfig,
+    KPDConfig,
+)
 from kernel_patches_daemon.github_logs import (
     BpfGithubLogExtractor,
     DefaultGithubLogExtractor,
@@ -143,20 +147,20 @@ async def get_mapped_branches(self, series: Series) -> List[str]:
         logging.info(f"Mapped to default branch order: {mapped_branches}")
         return mapped_branches
 
-    def close_existing_prs_with_same_base(
+    def close_existing_prs_for_series(
         self, workers: Sequence["BranchWorker"], pr: PullRequest
     ) -> None:
-        """Close existing pull requests with the same base, but different remote branch
+        """Close existing pull requests for the same series, but different target branch
 
         For given pull request, find all other pull requests with
-        the same base, but different remote branch and close them.
+        the same series name, but different remote branch and close them.
         """
 
         prs_to_close = [
             existing_pr
             for worker in workers
             for existing_pr in worker.prs.values()
-            if has_same_base_different_remote(pr.head.ref, existing_pr.head.ref)
+            if same_series_different_target(pr.head.ref, existing_pr.head.ref)
         ]
         # Remove matching PRs from other workers
         for pr_to_close in prs_to_close:
@@ -170,7 +174,7 @@ def close_existing_prs_with_same_base(
                     del worker.prs[pr_to_close.title]
 
     async def checkout_and_patch_safe(
-        self, worker, branch_name: str, series_to_apply: Series
+        self, worker: BranchWorker, branch_name: str, series_to_apply: Series
     ) -> Optional[PullRequest]:
         try:
             self.increment_counter("all_known_subjects")
@@ -221,6 +225,7 @@ async def sync_relevant_subject(self, subject: Subject) -> None:
                     continue
                 else:
                     logging.info(msg + "no more next, staying.")
+
             logging.info(f"Choosing branch {branch} to create/update PR.")
             pr = await self.checkout_and_patch_safe(worker, pr_branch_name, series)
             if pr is None:
@@ -231,7 +236,7 @@ async def sync_relevant_subject(self, subject: Subject) -> None:
             )
             await worker.sync_checks(pr, series)
             # Close out other PRs if exists
-            self.close_existing_prs_with_same_base(list(self.workers.values()), pr)
+            self.close_existing_prs_for_series(list(self.workers.values()), pr)
 
             break
         pass
@@ -285,19 +290,20 @@ async def sync_patches(self) -> None:
                     continue
 
                 if worker._is_relevant_pr(pr):
+                    parsed_ref = parse_pr_ref(pr.head.ref)
                     # ignore unknown format branch/PRs.
-                    if "/" not in pr.head.ref or HEAD_BASE_SEPARATOR not in pr.head.ref:
+                    if parsed_pr_ref_ok(parsed_ref):
                         continue
 
-                    series_id = int(get_base_branch_from_ref(pr.head.ref.split("/")[1]))
+                    series_id = parsed_ref["series_id"]
                     series = await self.pw.get_series_by_id(series_id)
                     subject = self.pw.get_subject_by_series(series)
                     if subject_name != subject.subject:
                         logger.warning(
                             f"Renaming {pr} from {subject_name} to {subject.subject} according to {series.id}"
                         )
                         pr.edit(title=subject.subject)
-                    branch_name = f"{await subject.branch}{HEAD_BASE_SEPARATOR}{worker.repo_branch}"
+                    branch_name = f"{await subject.branch}{SERIES_TARGET_SEPARATOR}{worker.repo_branch}"
                     latest_series = await subject.latest_series or series
                     pr = await self.checkout_and_patch_safe(
                         worker, branch_name, latest_series

kernel_patches_daemon/patchwork.py

@@ -23,6 +23,7 @@
 from aiohttp_retry import ExponentialRetry, RetryClient
 from cachetools import TTLCache
 
+from kernel_patches_daemon.config import SERIES_ID_SEPARATOR
 from kernel_patches_daemon.status import Status
 from multidict import MultiDict
 
@@ -293,7 +294,7 @@ async def branch(self) -> Optional[str]:
         relevant_series = await self.relevant_series
         if len(relevant_series) == 0:
             return None
-        return f"series/{relevant_series[0].id}"
+        return f"series{SERIES_ID_SEPARATOR}{relevant_series[0].id}"
 
     @property
     async def latest_series(self) -> Optional["Series"]:

tests/test_branch_worker.py

@@ -34,12 +34,15 @@
     email_in_submitter_allowlist,
     EmailBodyContext,
     furnish_ci_email_body,
-    has_same_base_different_remote,
-    HEAD_BASE_SEPARATOR,
+    same_series_different_target,
     temporary_patch_file,
     UPSTREAM_REMOTE_NAME,
 )
-from kernel_patches_daemon.config import EmailConfig
+from kernel_patches_daemon.config import (
+    EmailConfig,
+    SERIES_TARGET_SEPARATOR,
+    SERIES_ID_SEPARATOR,
+)
 from kernel_patches_daemon.github_logs import DefaultGithubLogExtractor
 from kernel_patches_daemon.patchwork import Series, Subject
 from kernel_patches_daemon.status import Status
@@ -486,15 +489,17 @@ class TestCase:
             TestCase(
                 name="A branch fetch pattern: expired should be deleted, not expired should not be deleted.",
                 branches=[
-                    "test1" + HEAD_BASE_SEPARATOR + self._bw.repo_branch,
-                    "test2" + HEAD_BASE_SEPARATOR + self._bw.repo_branch,
+                    f"series{SERIES_ID_SEPARATOR}111111{SERIES_TARGET_SEPARATOR}{self._bw.repo_branch}",
+                    f"series{SERIES_ID_SEPARATOR}222222{SERIES_TARGET_SEPARATOR}{self._bw.repo_branch}",
                 ],
                 fcp_called_branches=[
-                    "test1" + HEAD_BASE_SEPARATOR + self._bw.repo_branch,
-                    "test2" + HEAD_BASE_SEPARATOR + self._bw.repo_branch,
+                    f"series{SERIES_ID_SEPARATOR}111111{SERIES_TARGET_SEPARATOR}{self._bw.repo_branch}",
+                    f"series{SERIES_ID_SEPARATOR}222222{SERIES_TARGET_SEPARATOR}{self._bw.repo_branch}",
                 ],
                 fcp_return_prs=[PR(updated_at=expired_time), PR()],
-                deleted_branches=["test1" + HEAD_BASE_SEPARATOR + self._bw.repo_branch],
+                deleted_branches=[
+                    f"series{SERIES_ID_SEPARATOR}111111{SERIES_TARGET_SEPARATOR}{self._bw.repo_branch}"
+                ],
                 # filter_closed_prs for both "test1=>repo_branch", "test2=>repo_branch"
                 # only delete "test1=>repo_branch"
             ),
@@ -869,21 +874,23 @@ def test_create_color_labels(self) -> None:
             repo_mock.create_label.assert_not_called()
             repo_label_mock.edit.assert_called_once_with(name="label", color="00000")
 
-    def test_has_same_base_different_remote(self) -> None:
-        with self.subTest("same_base_different_remote"):
+    def test_same_series_different_target(self) -> None:
+        with self.subTest("same_series_different_target"):
             self.assertTrue(
-                has_same_base_different_remote("base=>remote", "base=>other_remote")
+                same_series_different_target(
+                    "series/1=>target", "series/1=>other_target"
+                )
             )
 
-        with self.subTest("different_base_same_remote"):
+        with self.subTest("different_series_same_target"):
             self.assertFalse(
-                has_same_base_different_remote("base=>remote", "other_base=>remote")
+                same_series_different_target("series/1=>target", "series/2=>target")
             )
 
-        with self.subTest("different_base_different_remote"):
+        with self.subTest("different_series_different_target"):
             self.assertFalse(
-                has_same_base_different_remote(
-                    "base=>remote", "other_base=>other_remote"
+                same_series_different_target(
+                    "series/1=>target", "series/2=>other_target"
                 )
             )
 

tests/test_github_sync.py

@@ -15,8 +15,8 @@
 from aioresponses import aioresponses
 
 from kernel_patches_daemon.branch_worker import NewPRWithNoChangeException
-from kernel_patches_daemon.config import KPDConfig
-from kernel_patches_daemon.github_sync import GithubSync, HEAD_BASE_SEPARATOR
+from kernel_patches_daemon.config import KPDConfig, SERIES_TARGET_SEPARATOR
+from kernel_patches_daemon.github_sync import GithubSync
 from tests.common.patchwork_mock import init_pw_responses, load_test_data, PatchworkMock
 
 TEST_BRANCH = "test-branch"
@@ -118,7 +118,7 @@ class TestCase:
                     gh.workers[TEST_BRANCH].ci_repo_dir.startswith(case.prefix),
                 )
 
-    def test_close_existing_prs_with_same_base(self) -> None:
+    def test_close_existing_prs_for_series(self) -> None:
         matching_pr_mock = MagicMock()
         matching_pr_mock.title = "matching"
         matching_pr_mock.head.ref = "test_branch=>remote_branch"
@@ -137,7 +137,7 @@ def test_close_existing_prs_with_same_base(self) -> None:
         input_pr_mock.head.ref = "test_branch=>other_remote_branch"
 
         workers = [copy.copy(branch_worker_mock) for _ in range(2)]
-        self._gh.close_existing_prs_with_same_base(workers, input_pr_mock)
+        self._gh.close_existing_prs_for_series(workers, input_pr_mock)
         for worker in workers:
             self.assertEqual(len(worker.prs), 1)
             self.assertTrue("irrelevant" in worker.prs)
@@ -202,7 +202,7 @@ async def test_sync_relevant_subject_no_mapped_branches(self) -> None:
 
     async def test_sync_relevant_subject_success_first_branch(self) -> None:
         series_prefix = "series/987654"
-        series_branch_name = f"{series_prefix}{HEAD_BASE_SEPARATOR}{TEST_BRANCH}"
+        series_branch_name = f"{series_prefix}{SERIES_TARGET_SEPARATOR}{TEST_BRANCH}"
 
         subject_mock, series_mock = self._setup_sync_relevant_subject_mocks()
         series_mock.all_tags = AsyncMock(return_value=["multibranch-tag"])
@@ -212,7 +212,7 @@ async def test_sync_relevant_subject_success_first_branch(self) -> None:
         pr_mock.head.ref = series_branch_name
 
         self._gh.checkout_and_patch_safe = AsyncMock(return_value=pr_mock)
-        self._gh.close_existing_prs_with_same_base = MagicMock()
+        self._gh.close_existing_prs_for_series = MagicMock()
 
         worker_mock = self._gh.workers[TEST_BRANCH]
         worker_mock.sync_checks = AsyncMock()
@@ -229,15 +229,17 @@ async def test_sync_relevant_subject_success_first_branch(self) -> None:
             worker_mock, series_branch_name, series_mock
         )
         worker_mock.sync_checks.assert_called_once_with(pr_mock, series_mock)
-        self._gh.close_existing_prs_with_same_base.assert_called_once_with(
+        self._gh.close_existing_prs_for_series.assert_called_once_with(
             list(self._gh.workers.values()), pr_mock
         )
 
     async def test_sync_relevant_subject_success_second_branch(self) -> None:
         """Test sync_relevant_subject when series fails on first branch but succeeds on second."""
         series_prefix = "series/333333"
-        bad_branch_name = f"{series_prefix}{HEAD_BASE_SEPARATOR}{TEST_BRANCH}"
-        good_branch_name = f"{series_prefix}{HEAD_BASE_SEPARATOR}{TEST_BPF_NEXT_BRANCH}"
+        bad_branch_name = f"{series_prefix}{SERIES_TARGET_SEPARATOR}{TEST_BRANCH}"
+        good_branch_name = (
+            f"{series_prefix}{SERIES_TARGET_SEPARATOR}{TEST_BPF_NEXT_BRANCH}"
+        )
 
         subject_mock, series_mock = self._setup_sync_relevant_subject_mocks()
         series_mock.all_tags = AsyncMock(return_value=["multibranch-tag"])
@@ -246,7 +248,7 @@ async def test_sync_relevant_subject_success_second_branch(self) -> None:
         pr_mock.head.ref = good_branch_name
 
         self._gh.checkout_and_patch_safe = AsyncMock(return_value=pr_mock)
-        self._gh.close_existing_prs_with_same_base = MagicMock()
+        self._gh.close_existing_prs_for_series = MagicMock()
 
         bad_worker_mock = self._gh.workers[TEST_BRANCH]
         bad_worker_mock.sync_checks = AsyncMock()
@@ -274,7 +276,7 @@ async def test_sync_relevant_subject_success_second_branch(self) -> None:
             good_worker_mock, good_branch_name, series_mock
         )
         good_worker_mock.sync_checks.assert_called_once_with(pr_mock, series_mock)
-        self._gh.close_existing_prs_with_same_base.assert_called_once_with(
+        self._gh.close_existing_prs_for_series.assert_called_once_with(
             list(self._gh.workers.values()), pr_mock
         )