github: pull requests pilot (bug 1989635)

src/lando/api/legacy/workers/landing_worker.py

@@ -36,6 +36,7 @@
 )
 from lando.pushlog.pushlog import PushLog, PushLogForRepo
 from lando.utils.config import read_lando_config
+from lando.utils.github import GitHubAPIClient
 from lando.utils.landing_checks import LandingChecks
 from lando.utils.tasks import phab_trigger_repo_update
 
@@ -120,6 +121,15 @@ def run_job(self, job: LandingJob) -> bool:
         job.set_landed_commit_ids()
         job.transition_status(JobAction.LAND, commit_id=commit_id)
 
+        if job.is_pull_request_job:
+            # TODO: move this to different method, and retry if needed.
+            # NOTE: This may need to happen on the revision-level when stack support is added.
+            pull_number = job.revisions.first().pull_number
+            message = f"Pull request closed by commit {commit_id}"
+            client = GitHubAPIClient(job.target_repo.url)
+            client.add_comment_to_pull_request(pull_number, message)
+            client.close_pull_request(pull_number)
+
         mots_path = Path(repo.path) / "mots.yaml"
         if mots_path.exists():
             logger.info(f"{mots_path} found, setting reviewer data.")
@@ -153,6 +163,30 @@ def run_job(self, job: LandingJob) -> bool:
 
         return True
 
+    def convert_patches_to_diff(self, scm: AbstractSCM, job: LandingJob):
+        """Generate a unified diff from multiple patches stored in a revision."""
+        # NOTE: this only applies to git patches that are downloaded from GitHub
+        # at this time. In theory this would work for any provided patches in a
+        # standard format.
+
+        # NOTE: this is only supported for jobs with a single revision at this time.
+        # See bug 2001185.
+
+        if len(job.revisions) > 1:
+            raise NotImplementedError(
+                "This method is not supported when job has more than 1 revision."
+            )
+        if len(job.revisions) == 0:
+            raise ValueError("No revisions found in job.")
+
+        revision = job.revisions[0]
+        if not revision.patches:
+            raise ValueError("Revision is missing patches.")
+
+        diff = scm.get_diff_from_patches(revision.patches)
+        revision.set_patch(f"{diff}\r\n")
+        revision.save()
+
     def apply_and_push(
         self,
         job: LandingJob,
@@ -164,7 +198,6 @@ def apply_and_push(
 
         Returns a tuple of bug_ids and tip commit_id.
         """
-        self.update_repo(repo, job, scm, job.target_commit_hash)
 
         def apply_patch(revision: Revision):
             logger.debug(f"Landing {revision} ...")
@@ -175,6 +208,12 @@ def apply_patch(revision: Revision):
                 revision.timestamp,
             )
 
+        self.update_repo(repo, job, scm, job.target_commit_hash)
+
+        if job.is_pull_request_job:
+            self.convert_patches_to_diff(scm, job)
+            self.update_repo(repo, job, scm, job.target_commit_hash)
+
         # Run through the patches one by one and try to apply them.
         logger.debug(
             f"About to land {job.revisions.count()} revisions: {job.revisions.all()} ..."

src/lando/api/tests/conftest.py

@@ -379,7 +379,7 @@ def mock_permissions():
 
 
 @pytest.fixture
-def proxy_client(monkeypatch, fake_request):
+def proxy_client(monkeypatch, fake_request, mock_response):
     """A client that bridges tests designed to work with the API.
 
     Most tests that use the API no longer need to access those endpoints through
@@ -391,18 +391,6 @@ def proxy_client(monkeypatch, fake_request):
     reimplemented to not need a response or response-like object.
     """
 
-    class MockResponse:
-        """Mock response class to satisfy some requirements of tests."""
-
-        # NOTE: The methods tested that rely on this class should be reimplemented
-        # to no longer need the structure of a response to function.
-        def __init__(self, status_code=200, json=None):
-            self.json = json or {}
-            self.status_code = status_code
-            self.content_type = (
-                "application/json" if status_code < 400 else "application/problem+json"
-            )
-
     class ProxyClient:
         request = fake_request()
 
@@ -419,7 +407,8 @@ def _handle__get__stacks__id(self, path):
             # and isn't required in the proxy client tests.
             json_response.pop("stack")
 
-            return MockResponse(json=json.loads(json.dumps(json_response)))
+            # The double encode/decode is to coerce Python tuples to lists.
+            return mock_response(json_dict=json.loads(json.dumps(json_response)))
 
         def _handle__get__transplants__id(self, path):
             stack_revision_id = path.removeprefix("/transplants?stack_revision_id=")
@@ -430,16 +419,16 @@ def _handle__get__transplants__id(self, path):
                 # For these endpoints, some responses contain different status codes
                 # which are represented as the second item in a tuple.
                 json_response, status_code = result
-                return MockResponse(
-                    json=json.loads(json.dumps(json_response)),
+                return mock_response(
+                    json_dict=json.loads(json.dumps(json_response)),
                     status_code=status_code,
                 )
             # In the rest of the cases, the returned result is a response object.
             return result
 
         def _handle__post__transplants__dryrun(self, **kwargs):
             json_response = legacy_api_transplants.dryrun(self.request, kwargs["json"])
-            return MockResponse(json=json.loads(json.dumps(json_response)))
+            return mock_response(json_dict=json.loads(json.dumps(json_response)))
 
         def _handle__post__transplants(self, path, **kwargs):
             try:
@@ -449,23 +438,23 @@ def _handle__post__transplants(self, path, **kwargs):
             except LegacyAPIException as e:
                 # Handle exceptions and pass along the status code to the response object.
                 if e.extra:
-                    return MockResponse(json=e.extra, status_code=e.status)
+                    return mock_response(json_dict=e.extra, status_code=e.status)
                 if e.json_detail:
-                    return MockResponse(json=e.json_detail, status_code=e.status)
-                return MockResponse(json=e.args, status_code=e.status)
+                    return mock_response(json_dict=e.json_detail, status_code=e.status)
+                return mock_response(json_dict=e.args, status_code=e.status)
             except Exception as e:
                 # TODO: double check that this is a thing in legacy?
                 # Added this due to a validation error (test_transplant_wrong_landing_path_format)
-                return MockResponse(json=[f"error ({e})"], status_code=400)
-            return MockResponse(
-                json=json.loads(json.dumps(json_response)), status_code=status_code
+                return mock_response(json_dict=[f"error ({e})"], status_code=400)
+            return mock_response(
+                json_dict=json.loads(json.dumps(json_response)), status_code=status_code
             )
 
         def _handle__put__landing_jobs__id(self, path, **kwargs):
             job_id = int(path.removeprefix("/landing_jobs/"))
             landing_job_api = LandingJobApiView()
             response = landing_job_api.put(self.request, job_id)
-            return MockResponse(json=json.loads(response.content))
+            return mock_response(json_dict=json.loads(response.content))
 
         def get(self, path, *args, **kwargs):
             """Handle various get endpoints."""

src/lando/api/tests/test_hooks.py

@@ -65,8 +65,8 @@ def treestatus_exception2():
 
     response = client.get("__testing__/treestatus_exception1")
     assert response.status_code == 500
-    assert response.json["title"] == "Tree Status Error"
+    assert response.json()["title"] == "Tree Status Error"
 
     response = client.get("__testing__/treestatus_exception2")
     assert response.status_code == 500
-    assert response.json["title"] == "Tree Status Error"
+    assert response.json()["title"] == "Tree Status Error"

src/lando/api/tests/test_revisions.py

@@ -42,7 +42,7 @@ def test_secure_api_flag_on_public_revision_is_false(
 
     response = proxy_client.get("/stacks/D{}".format(revision["id"]))
     assert response.status_code == 200
-    response_revision = response.json["revisions"].pop()
+    response_revision = response.json()["revisions"].pop()
     assert not response_revision["is_secure"]
 
 
@@ -61,7 +61,7 @@ def test_secure_api_flag_on_secure_revision_is_true(
     response = proxy_client.get("/stacks/D{}".format(revision["id"]))
 
     assert response.status_code == 200
-    response_revision = response.json["revisions"].pop()
+    response_revision = response.json()["revisions"].pop()
     assert response_revision["is_secure"]
 
 

src/lando/api/tests/test_stacks.py

@@ -808,18 +808,18 @@ def test_integrated_stack_endpoint_simple(
     response = proxy_client.get("/stacks/D{}".format(r3["id"]))
     assert response.status_code == 200
 
-    assert len(response.json["edges"]) == 4
-    assert [r2["phid"], r1["phid"]] in response.json["edges"]
-    assert [r3["phid"], r1["phid"]] in response.json["edges"]
-    assert [r4["phid"], r2["phid"]] in response.json["edges"]
-    assert [r4["phid"], r3["phid"]] in response.json["edges"]
-
-    assert len(response.json["landable_paths"]) == 2
-    assert [r1["phid"], r2["phid"]] in response.json["landable_paths"]
-    assert [r1["phid"], r3["phid"]] in response.json["landable_paths"]
-
-    assert len(response.json["revisions"]) == 4
-    revisions = {r["phid"]: r for r in response.json["revisions"]}
+    assert len(response.json()["edges"]) == 4
+    assert [r2["phid"], r1["phid"]] in response.json()["edges"]
+    assert [r3["phid"], r1["phid"]] in response.json()["edges"]
+    assert [r4["phid"], r2["phid"]] in response.json()["edges"]
+    assert [r4["phid"], r3["phid"]] in response.json()["edges"]
+
+    assert len(response.json()["landable_paths"]) == 2
+    assert [r1["phid"], r2["phid"]] in response.json()["landable_paths"]
+    assert [r1["phid"], r3["phid"]] in response.json()["landable_paths"]
+
+    assert len(response.json()["revisions"]) == 4
+    revisions = {r["phid"]: r for r in response.json()["revisions"]}
     assert r1["phid"] in revisions
     assert r2["phid"] in revisions
     assert r3["phid"] in revisions
@@ -849,9 +849,9 @@ def test_integrated_stack_endpoint_repos(
     response = proxy_client.get("/stacks/D{}".format(r4["id"]))
     assert response.status_code == 200
 
-    assert len(response.json["repositories"]) == 2
+    assert len(response.json()["repositories"]) == 2
 
-    repositories = {r["phid"]: r for r in response.json["repositories"]}
+    repositories = {r["phid"]: r for r in response.json()["repositories"]}
     assert repo["phid"] in repositories
     assert unsupported_repo["phid"] in repositories
     assert repositories[repo["phid"]]["landing_supported"]
@@ -881,7 +881,7 @@ def test_integrated_stack_has_revision_security_status(
     response = proxy_client.get("/stacks/D{}".format(secure_revision["id"]))
     assert response.status_code == 200
 
-    revisions = {r["phid"]: r for r in response.json["revisions"]}
+    revisions = {r["phid"]: r for r in response.json()["revisions"]}
     assert not revisions[public_revision["phid"]]["is_secure"]
     assert revisions[secure_revision["phid"]]["is_secure"]
 
@@ -905,8 +905,8 @@ def test_integrated_stack_response_mismatch_returns_404(
 
     response = proxy_client.get("/stacks/D{}".format(r1["id"]))
     assert response.status_code == 200
-    assert len(response.json["edges"]) == 1
-    assert len(response.json["revisions"]) == 2
+    assert len(response.json()["edges"]) == 1
+    assert len(response.json()["revisions"]) == 2
 
     # Remove r2 from the response.
     phabdouble._revisions = [
@@ -921,8 +921,8 @@ def test_integrated_stack_response_mismatch_returns_404(
 
     response = proxy_client.get("/stacks/D{}".format(r1["id"]))
     assert response.status_code == 200
-    assert len(response.json["edges"]) == 0
-    assert len(response.json["revisions"]) == 1
+    assert len(response.json()["edges"]) == 0
+    assert len(response.json()["revisions"]) == 1
 
 
 def test_revisionstack_single():