Depend on Phabricator build target ID in trigger mode (#2611)

* Depend on Phabricator build target ID in trigger mode * Fix tests --------- Co-authored-by: Bastien Abadie <[email protected]>
mozilla · Feb 11, 2025 · bab9819 · bab9819
1 parent 1692b12
commit bab9819
Show file tree

Hide file tree

Showing 6 changed files with 49 additions and 109 deletions.
diff --git a/bot/code_review_bot/cli.py b/bot/code_review_bot/cli.py
@@ -163,10 +163,9 @@ def main():
             revision = Revision.from_decision_task(
                 queue_service.task(settings.mozilla_central_group_id), phabricator_api
             )
-        elif settings.phabricator_revision_phid:
+        elif settings.phabricator_build_target:
             revision = Revision.from_phabricator_trigger(
-                settings.phabricator_revision_phid,
-                settings.phabricator_transactions,
+                settings.phabricator_build_target,
                 phabricator_api,
             )
         else:
@@ -207,7 +206,7 @@ def main():
             w.ingest_revision(revision, settings.autoland_group_id)
         elif settings.mozilla_central_group_id:
             w.ingest_revision(revision, settings.mozilla_central_group_id)
-        elif settings.phabricator_revision_phid:
+        elif settings.phabricator_build_target:
             w.start_analysis(revision)
         else:
             w.run(revision)

diff --git a/bot/code_review_bot/config.py b/bot/code_review_bot/config.py
@@ -6,7 +6,6 @@
 import atexit
 import collections
 import fnmatch
-import json
 import os
 import shutil
 import tempfile
@@ -48,8 +47,7 @@ def __init__(self):
         self.try_group_id = None
         self.autoland_group_id = None
         self.mozilla_central_group_id = None
-        self.phabricator_revision_phid = None
-        self.phabricator_transactions = None
+        self.phabricator_build_target = None
         self.repositories = []
         self.decision_env_prefixes = []
 
@@ -90,22 +88,12 @@ def setup(
             self.autoland_group_id = os.environ["AUTOLAND_TASK_GROUP_ID"]
         elif "MOZILLA_CENTRAL_TASK_GROUP_ID" in os.environ:
             self.mozilla_central_group_id = os.environ["MOZILLA_CENTRAL_TASK_GROUP_ID"]
-        elif (
-            "PHABRICATOR_OBJECT_PHID" in os.environ
-            and "PHABRICATOR_TRANSACTIONS" in os.environ
-        ):
+        elif "PHABRICATOR_BUILD_TARGET" in os.environ:
             # Setup trigger mode using Phabricator information
-            self.phabricator_revision_phid = os.environ["PHABRICATOR_OBJECT_PHID"]
-            assert self.phabricator_revision_phid.startswith(
-                "PHID-DREV"
-            ), f"Not a phabrication revision PHID: {self.phabricator_revision_phid}"
-            try:
-                self.phabricator_transactions = json.loads(
-                    os.environ["PHABRICATOR_TRANSACTIONS"]
-                )
-            except Exception as e:
-                logger.error("Failed to parse phabricator transactions", err=str(e))
-                raise
+            self.phabricator_build_target = os.environ["PHABRICATOR_BUILD_TARGET"]
+            assert self.phabricator_build_target.startswith(
+                "PHID-HMBT"
+            ), f"Not a phabrication build target PHID: {self.phabricator_build_target}"
         else:
             raise Exception("Only TRY mode is supported")
 

diff --git a/bot/code_review_bot/revisions.py b/bot/code_review_bot/revisions.py
@@ -7,7 +7,6 @@
 import urllib.parse
 from datetime import timedelta
 from pathlib import Path
-from typing import List
 
 import requests
 import rs_parsepatch
@@ -314,9 +313,23 @@ def from_decision_task(task: dict, phabricator: PhabricatorAPI):
         )
 
     @staticmethod
-    def from_phabricator_trigger(
-        revision_phid: str, transactions: List[str], phabricator: PhabricatorAPI
-    ):
+    def from_phabricator_trigger(build_target_phid: str, phabricator: PhabricatorAPI):
+        assert build_target_phid.startswith("PHID-HMBT-")
+        buildable = phabricator.find_target_buildable(build_target_phid)
+        diff_phid = buildable["fields"]["objectPHID"]
+        assert diff_phid.startswith("PHID-DIFF-")
+
+        # Load diff details to get the diff revision
+        # We also load the commits list in order to get the email of the author of the
+        # patch for sending email if builds are failing.
+        diffs = phabricator.search_diffs(
+            diff_phid=diff_phid, attachments={"commits": True}
+        )
+        assert len(diffs) == 1, f"No diff available for {diff_phid}"
+        diff = diffs[0]
+        logger.info("Found diff", id=diff["id"], phid=diff["phid"])
+        revision_phid = diff["revisionPHID"]
+
         # Load revision details from Phabricator
         revision = phabricator.load_revision(revision_phid)
         logger.info("Found revision", id=revision["id"], phid=revision["phid"])
@@ -339,79 +352,11 @@ def from_phabricator_trigger(
             )
         logger.info("Found repository", name=repo_name, phid=repo_phid)
 
-        # Lookup transactions to find Diff
-        response = phabricator.request(
-            "transaction.search", constraints={"phids": transactions}, objectType="DREV"
-        )
-        diff_phid = None
-        for transaction in response["data"]:
-            fields = transaction["fields"]
-            if not fields:
-                continue
-            new = fields.get("new", "")
-            if new.startswith("PHID-DIFF-"):
-                diff_phid = new
-                break
-
-        # Check a diff is found in transactions or use last diff available
-        if diff_phid is None:
-            diffs = phabricator.search_diffs(
-                revision_phid=revision_phid,
-                attachments={"commits": True},
-                order="newest",
-            )
-            if not diffs:
-                raise Exception(f"No diff found on revision {revision_phid}")
-            diff = diffs[0]
-            diff_phid = diff["phid"]
-            logger.info(
-                "Using most recent diff on revision", id=diff["id"], phid=diff["phid"]
-            )
-
-        else:
-            # Load diff details to get the diff revision
-            # We also load the commits list in order to get the email of the author of the
-            # patch for sending email if builds are failing.
-            diffs = phabricator.search_diffs(
-                diff_phid=diff_phid, attachments={"commits": True}
-            )
-            assert len(diffs) == 1, f"No diff available for {diff_phid}"
-            diff = diffs[0]
-            logger.info("Found diff from transaction", id=diff["id"], phid=diff["phid"])
-
-        # Lookup harbormaster target passing through Buildable, then Build, finally Build Target
-        out = phabricator.request(
-            "harbormaster.buildable.search",
-            constraints={"containerPHIDs": [revision_phid], "objectPHIDs": [diff_phid]},
-        )
-        assert len(out["data"]) == 1
-        buildable_phid = out["data"][0]["phid"]
-        logger.info("Found Harbormaster buildable", phid=buildable_phid)
-
-        out = phabricator.request(
-            "harbormaster.build.search", constraints={"buildables": [buildable_phid]}
-        )
-        assert len(out["data"]) == 1
-        build_phid = out["data"][0]["phid"]
-        logger.info("Found Harbormaster build", phid=build_phid)
-
-        out = phabricator.request(
-            "harbormaster.target.search", constraints={"buildPHIDs": [build_phid]}
-        )
-        if len(out["data"]) == 1:
-            build_target_phid = out["data"][0]["phid"]
-            logger.info("Found Harbormaster build target", phid=build_target_phid)
-        else:
-            build_target_phid = None
-            logger.warning(
-                "No build target found on Phabricator, no updates will be published"
-            )
-
         return Revision(
             phabricator_id=revision["id"],
             phabricator_phid=revision_phid,
             diff_id=diff["id"],
-            diff_phid=diff_phid,
+            diff_phid=diff["phid"],
             diff=diff,
             build_target_phid=build_target_phid,
             url="https://{}/D{}".format(phabricator.hostname, revision["id"]),

diff --git a/bot/code_review_bot/workflow.py b/bot/code_review_bot/workflow.py
@@ -285,7 +285,7 @@ def start_analysis(self, revision):
             phabricator.update_state(build)
 
             # Continue with workflow once the build is public
-            if build.state == PhabricatorBuildState.Public:
+            if build.state is PhabricatorBuildState.Public:
                 break
 
             # Retry later if the build is not yet seen as public
@@ -296,6 +296,10 @@ def start_analysis(self, revision):
             )
             time.sleep(30)
 
+        # Make sure the build is now public
+        if build.state is not PhabricatorBuildState.Public:
+            raise Exception("Cannot process private builds")
+
         # When the build is public, load needed details
         try:
             phabricator.load_patches_stack(build)

diff --git a/bot/tests/mocks/phabricator_revision_search.json b/bot/tests/mocks/phabricator_revision_search.json
@@ -25,7 +25,13 @@
                     },
                     "bugzilla.bug-id": "1234567"
                 },
-                "attachments": {}
+                "attachments": {
+                    "projects": {
+                        "projectPHIDs": [
+                            "PHID-PROJ-A"
+                        ]
+                    }
+                }
             }
         ],
         "maps": {},

diff --git a/bot/tests/test_analysis.py b/bot/tests/test_analysis.py
@@ -6,6 +6,7 @@
 import tempfile
 
 from libmozevent import utils
+from libmozevent.phabricator import PhabricatorActions
 
 from code_review_bot.config import RepositoryConf
 from code_review_bot.revisions import Revision
@@ -19,10 +20,7 @@ def test_revision(mock_phabricator):
 
     with mock_phabricator as api:
         revision = Revision.from_phabricator_trigger(
-            revision_phid="PHID-DREV-1234",
-            transactions=[
-                "PHID-XACT-aaaa",
-            ],
+            build_target_phid="PHID-HMBT-test",
             phabricator=api,
         )
 
@@ -31,20 +29,20 @@ def test_revision(mock_phabricator):
         "base_repository": "https://hg.mozilla.org/mozilla-central",
         "bugzilla_id": 1234567,
         "diff_id": 42,
-        "diff_phid": "PHID-DIFF-test",
+        "diff_phid": "PHID-DIFF-testABcd12",
         "has_clang_files": False,
         "head_changeset": None,
         "head_repository": None,
         "id": 51,
         "mercurial_revision": None,
-        "phid": "PHID-DREV-1234",
+        "phid": "PHID-DREV-zzzzz",
         "repository": None,
         "target_repository": "https://hg.mozilla.org/mozilla-central",
         "title": "Static Analysis tests",
         "url": "https://phabricator.test/D51",
     }
     assert revision.build_target_phid == "PHID-HMBT-test"
-    assert revision.phabricator_phid == "PHID-DREV-1234"
+    assert revision.phabricator_phid == "PHID-DREV-zzzzz"
     assert revision.base_repository_conf == RepositoryConf(
         name="mozilla-central",
         try_name="try",
@@ -81,6 +79,9 @@ def mock_hgrun(cmd):
 
     monkeypatch.setattr(utils, "hg_run", mock_hgrun)
 
+    # Build never expires otherwise the analysis stops early
+    monkeypatch.setattr(PhabricatorActions, "is_expired_build", lambda _, build: False)
+
     # Control ssh key destination
     ssh_key_path = tmpdir / "ssh.key"
     monkeypatch.setattr(tempfile, "mkstemp", lambda suffix: (None, ssh_key_path))
@@ -94,10 +95,7 @@ def mock_hgrun(cmd):
         mock_workflow.phabricator = api
 
         revision = Revision.from_phabricator_trigger(
-            revision_phid="PHID-DREV-1234",
-            transactions=[
-                "PHID-XACT-aaaa",
-            ],
+            build_target_phid="PHID-HMBT-test",
             phabricator=api,
         )
 
@@ -197,7 +195,7 @@ def mock_hgrun(cmd):
         (
             "commit",
             {
-                "message": "try_task_config for code-review\n"
+                "message": "try_task_config for https://phabricator.test/D51\n"
                 "Differential Diff: PHID-DIFF-testABcd12",
                 "user": "libmozevent <[email protected]>",
             },