launchpad-reviewers team mailing list archive

[Ioana Lasc <ioana.lasc@xxxxxxxxxxxxx>]

Ioana Lasc has proposed merging ~ilasc/launchpad:add-git-gc-api into launchpad:master.

Commit message:
Add endpoint to call Git GC

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/401221

Add a new endpoint to the exiting IGitRepositoryExpensiveRequest to enable us to trigger an async GC run in Turnip.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilasc/launchpad:add-git-gc-api into launchpad:master.

diff --git a/lib/lp/code/errors.py b/lib/lp/code/errors.py
index 6406a55..213d112 100644
--- a/lib/lp/code/errors.py
+++ b/lib/lp/code/errors.py
@@ -26,6 +26,7 @@ __all__ = [
     'CannotHaveLinkedBranch',
     'CannotModifyNonHostedGitRepository',
     'CannotRepackRepository',
+    'CannotRunGitGC',
     'CannotUpgradeBranch',
     'CannotUpgradeNonHosted',
     'CodeImportAlreadyRequested',
@@ -495,6 +496,10 @@ class CannotRepackRepository(Exception):
     """Raised when there was a failure to repack a repository."""
 
 
+class CannotRunGitGC(Exception):
+    """Raised when there was a failure to run git gc for a repository."""
+
+
 class GitRepositoryDeletionFault(Exception):
     """Raised when there is a fault deleting a repository."""
 
diff --git a/lib/lp/code/interfaces/githosting.py b/lib/lp/code/interfaces/githosting.py
index 3a8585d..cb316af 100644
--- a/lib/lp/code/interfaces/githosting.py
+++ b/lib/lp/code/interfaces/githosting.py
@@ -156,3 +156,11 @@ class IGitHostingClient(Interface):
             service.
         :param logger: An optional logger.
         """
+
+    def runGitGC(path, logger=None):
+        """Run Git GC for a repository.
+
+        :param path: Physical path of the new repository on the hosting
+            service.
+        :param logger: An optional logger.
+        """
diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
index 5374fb7..f81d780 100644
--- a/lib/lp/code/interfaces/gitrepository.py
+++ b/lib/lp/code/interfaces/gitrepository.py
@@ -772,6 +772,14 @@ class IGitRepositoryExpensiveRequest(Interface):
         Raises Unauthorized if the repack was attempted by a person
         that is not an admin or a registry expert."""
 
+    @export_write_operation()
+    @operation_for_version("devel")
+    def runGitGC():
+        """Trigger a gc run for a given git repository.
+
+        Raises Unauthorized if the repack was attempted by a person
+        that is not an admin or a registry expert."""
+
 
 class IGitRepositoryEdit(IWebhookTarget):
     """IGitRepository methods that require launchpad.Edit permission."""
diff --git a/lib/lp/code/model/githosting.py b/lib/lp/code/model/githosting.py
index ea0f893..baaec47 100644
--- a/lib/lp/code/model/githosting.py
+++ b/lib/lp/code/model/githosting.py
@@ -28,6 +28,7 @@ from zope.interface import implementer
 
 from lp.code.errors import (
     CannotRepackRepository,
+    CannotRunGitGC,
     GitReferenceDeletionFault,
     GitRepositoryBlobNotFound,
     GitRepositoryCreationFault,
@@ -324,3 +325,18 @@ class GitHostingClient:
             raise CannotRepackRepository(
                 "Failed to repack Git repository %s: %s" %
                 (path, six.text_type(e)))
+
+    def runGitGC(self, path, logger=None):
+        """See `IGitHostingClient`."""
+
+        url = "/repo/%s/gc" % path
+        try:
+            if logger is not None:
+                logger.info(
+                    "Running gc for repository %s" % (
+                        path))
+            return self._post(url)
+        except requests.RequestException as e:
+            raise CannotRunGitGC(
+                "Failed to run Git GC for repository %s: %s" %
+                (path, six.text_type(e)))
diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
index 2c66571..b1f95a9 100644
--- a/lib/lp/code/model/gitrepository.py
+++ b/lib/lp/code/model/gitrepository.py
@@ -494,6 +494,9 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
         getUtility(IGitHostingClient).repackRepository(self.getInternalPath())
         self.date_last_repacked = UTC_NOW
 
+    def runGitGC(self):
+        getUtility(IGitHostingClient).runGitGC(self.getInternalPath())
+
     @property
     def namespace(self):
         """See `IGitRepository`."""
diff --git a/lib/lp/code/model/tests/test_githosting.py b/lib/lp/code/model/tests/test_githosting.py
index 2f38c5d..9202459 100644
--- a/lib/lp/code/model/tests/test_githosting.py
+++ b/lib/lp/code/model/tests/test_githosting.py
@@ -39,6 +39,7 @@ from zope.security.proxy import removeSecurityProxy
 
 from lp.code.errors import (
     CannotRepackRepository,
+    CannotRunGitGC,
     GitReferenceDeletionFault,
     GitRepositoryBlobNotFound,
     GitRepositoryCreationFault,
@@ -502,3 +503,16 @@ class TestGitHostingClient(TestCase):
                 "Failed to repack Git repository /repo/123: "
                 "400 Client Error: Bad Request",
                 self.client.repackRepository, "/repo/123")
+
+    def test_git_gc(self):
+        with self.mockRequests("POST", status=200):
+            gc = self.client.runGitGC("/repo/123")
+        self.assertEqual(None, gc)
+
+    def test_git_gc_failure(self):
+        with self.mockRequests("POST", status=400):
+            self.assertRaisesWithContent(
+                CannotRunGitGC,
+                "Failed to run Git GC for repository /repo/123: "
+                "400 Client Error: Bad Request",
+                self.client.runGitGC, "/repo/123")
diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
index 5790338..1dbe329 100644
--- a/lib/lp/code/model/tests/test_gitrepository.py
+++ b/lib/lp/code/model/tests/test_gitrepository.py
@@ -3929,6 +3929,50 @@ class TestGitRepositoryWebservice(TestCaseWithFactory):
             'date_last_scanned': Equals(UTC_NOW),
             }))
 
+    def test_git_gc_owner(self):
+        # Repository owner cannot request a git GC run
+        hosting_fixture = self.useFixture(GitHostingFixture())
+        owner_db = self.factory.makePerson()
+        repository_db = self.factory.makeGitRepository(
+            owner=owner_db, name="repository")
+
+        with person_logged_in(owner_db):
+            self.assertRaises(
+                Unauthorized, getattr, repository_db, 'runGitGC')
+        self.assertEqual(0, hosting_fixture.runGitGC.call_count)
+
+    def test_git_gc_admin(self):
+        # Admins can trigger a git GC run
+        hosting_fixture = self.useFixture(GitHostingFixture())
+        owner_db = self.factory.makePerson()
+        repository_db = self.factory.makeGitRepository(
+            owner=owner_db, name="repository")
+        admin = getUtility(ILaunchpadCelebrities).admin.teamowner
+        with person_logged_in(admin):
+            repository_db.runGitGC()
+        self.assertEqual(
+            [((repository_db.getInternalPath(),), {})],
+            hosting_fixture.runGitGC.calls)
+        self.assertEqual(1, hosting_fixture.runGitGC.call_count)
+
+    def test_git_gc_registry_expert(self):
+        # Registry experts can trigger a Git GC run
+        hosting_fixture = self.useFixture(GitHostingFixture())
+        admin = getUtility(ILaunchpadCelebrities).admin.teamowner
+        person = self.factory.makePerson()
+        owner_db = self.factory.makePerson()
+        repository_db = self.factory.makeGitRepository(
+            owner=owner_db, name="repository")
+        with admin_logged_in():
+            getUtility(ILaunchpadCelebrities).registry_experts.addMember(
+                person, admin)
+        with person_logged_in(person):
+            repository_db.runGitGC()
+        self.assertEqual(
+            [((repository_db.getInternalPath(),), {})],
+            hosting_fixture.runGitGC.calls)
+        self.assertEqual(1, hosting_fixture.runGitGC.call_count)
+
     def test_urls(self):
         owner_db = self.factory.makePerson(name="person")
         project_db = self.factory.makeProduct(name="project")
diff --git a/lib/lp/code/tests/helpers.py b/lib/lp/code/tests/helpers.py
index 42d1161..b0a2bfc 100644
--- a/lib/lp/code/tests/helpers.py
+++ b/lib/lp/code/tests/helpers.py
@@ -335,6 +335,7 @@ class GitHostingFixture(fixtures.Fixture):
         self.delete = FakeMethod()
         self.disable_memcache = disable_memcache
         self.repackRepository = FakeMethod()
+        self.runGitGC = FakeMethod()
 
     def _setUp(self):
         self.useFixture(ZopeUtilityFixture(self, IGitHostingClient))