Launchpad itself

Merge ~ilasc/launchpad:add-repack-endpoint into launchpad:master

  1. Git
  2. lp:~ilasc/launchpad
  3. add-repack-endpoint
  4. Merge into master
Proposed by Ioana Lasc
Status: Merged
Approved by: Ioana Lasc
Approved revision: 670b326afc287ffe59fb54cc349c25f4810fec10
Merge reported by: Otto Co-Pilot
Merged at revision: not available
Proposed branch: ~ilasc/launchpad:add-repack-endpoint
Merge into: launchpad:master
Diff against target: 264 lines (+120/-2)
10 files modified
lib/lp/code/configure.zcml (+3/-0)
lib/lp/code/errors.py (+5/-0)
lib/lp/code/interfaces/githosting.py (+8/-0)
lib/lp/code/interfaces/gitrepository.py (+16/-1)
lib/lp/code/model/githosting.py (+16/-1)
lib/lp/code/model/gitrepository.py (+3/-0)
lib/lp/code/model/tests/test_githosting.py (+14/-0)
lib/lp/code/model/tests/test_gitrepository.py (+44/-0)
lib/lp/code/tests/helpers.py (+1/-0)
lib/lp/security.py (+10/-0)
Reviewer Review Type Date Requested Status
Colin Watson (community) Approve
Thiago F. Pappacena (community) Approve
Review via email: mp+395148@code.launchpad.net

Commit message

Add endpoint for git repository repack

To post a comment you must log in.
Revision history for this message
Thiago F. Pappacena (pappacena) wrote :

LGTM. Just a couple minor comments.

review: Approve
Revision history for this message
Ioana Lasc (ilasc) wrote :

Thank you Thiago!
Fully agreed with your comments and addressed all.
Also added a new test and handling for case when the repo passed in by the user is not found.

Revision history for this message
Colin Watson (cjwatson) wrote :

There are a few things I'd like to see laid out differently, but it should be good to go after that.

review: Needs Fixing
Revision history for this message
Ioana Lasc (ilasc) wrote :

Very constructive comments as usual, thanks Colin!
I believe this now has all the necessary elements.
Ready for another look.

Revision history for this message
Colin Watson (cjwatson) wrote :

Nearly there, let's just remove the unnecessary lookup which causes confusion ...

review: Needs Fixing
Revision history for this message
Ioana Lasc (ilasc) wrote :

Definitely agreed on removing the unnecessary lookup from IGitRepository and the other aspects you mentioned inline.
Thanks Colin, MP ready for another look now.

Revision history for this message
Colin Watson (cjwatson) :
review: Approve
Revision history for this message
Ioana Lasc (ilasc) :
Revision history for this message
Colin Watson (cjwatson) :
Revision history for this message
Colin Watson (cjwatson) :
Revision history for this message
Ioana Lasc (ilasc) wrote :

Thank you Colin, now it makes sense why the first approach was failing in my unit test.
Both suggested changes incorporated and test suite passes for "git", landing now.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/lib/lp/code/configure.zcml b/lib/lp/code/configure.zcml
2index 898e645..61c18c1 100644
3--- a/lib/lp/code/configure.zcml
4+++ b/lib/lp/code/configure.zcml
5@@ -836,6 +836,9 @@
6 permission="launchpad.Edit"
7 interface="lp.code.interfaces.gitrepository.IGitRepositoryEdit"
8 set_schema="lp.code.interfaces.gitrepository.IGitRepositoryEditableAttributes" />
9+ <require
10+ permission="launchpad.ExpensiveRequest"
11+ interface="lp.code.interfaces.gitrepository.IGitRepositoryExpensiveRequest"/>
12 </class>
13 <adapter
14 for="lp.code.interfaces.gitrepository.IGitRepository"
15diff --git a/lib/lp/code/errors.py b/lib/lp/code/errors.py
16index bed7db5..6406a55 100644
17--- a/lib/lp/code/errors.py
18+++ b/lib/lp/code/errors.py
19@@ -25,6 +25,7 @@ __all__ = [
20 'CannotDeleteGitRepository',
21 'CannotHaveLinkedBranch',
22 'CannotModifyNonHostedGitRepository',
23+ 'CannotRepackRepository',
24 'CannotUpgradeBranch',
25 'CannotUpgradeNonHosted',
26 'CodeImportAlreadyRequested',
27@@ -490,6 +491,10 @@ class GitRepositoryBlobUnsupportedRemote(Exception):
28 self.repository_url)
29
30
31+class CannotRepackRepository(Exception):
32+ """Raised when there was a failure to repack a repository."""
33+
34+
35 class GitRepositoryDeletionFault(Exception):
36 """Raised when there is a fault deleting a repository."""
37
38diff --git a/lib/lp/code/interfaces/githosting.py b/lib/lp/code/interfaces/githosting.py
39index 441e850..3a8585d 100644
40--- a/lib/lp/code/interfaces/githosting.py
41+++ b/lib/lp/code/interfaces/githosting.py
42@@ -148,3 +148,11 @@ class IGitHostingClient(Interface):
43 :param refs: A list of tuples like (repo_path, ref_name) to be deleted.
44 :param logger: An optional logger.
45 """
46+
47+ def repackRepository(path, logger=None):
48+ """Repack a Git repository.
49+
50+ :param path: Physical path of the new repository on the hosting
51+ service.
52+ :param logger: An optional logger.
53+ """
54diff --git a/lib/lp/code/interfaces/gitrepository.py b/lib/lp/code/interfaces/gitrepository.py
55index ed17183..8c90059 100644
56--- a/lib/lp/code/interfaces/gitrepository.py
57+++ b/lib/lp/code/interfaces/gitrepository.py
58@@ -12,6 +12,7 @@ __all__ = [
59 'git_repository_name_validator',
60 'IGitRepository',
61 'IGitRepositoryDelta',
62+ 'IGitRepositoryExpensiveRequest',
63 'IGitRepositorySet',
64 'IHasGitRepositoryURL',
65 'user_has_special_git_repository_access',
66@@ -737,6 +738,20 @@ class IGitRepositoryEditableAttributes(Interface):
67 "refs/heads/master.")))
68
69
70+class IGitRepositoryExpensiveRequest(Interface):
71+ """IGitRepository methods that require
72+ launchpad.ExpensiveRequest permission.
73+ """
74+
75+ @export_write_operation()
76+ @operation_for_version("devel")
77+ def repackRepository():
78+ """Trigger a repack repository operation.
79+
80+ Raises Unauthorized if the repack was attempted by a person
81+ that is not an admin or a registry expert."""
82+
83+
84 class IGitRepositoryEdit(IWebhookTarget):
85 """IGitRepository methods that require launchpad.Edit permission."""
86
87@@ -937,7 +952,7 @@ class IGitRepositoryEdit(IWebhookTarget):
88 @exported_as_webservice_entry(plural_name="git_repositories", as_of="beta")
89 class IGitRepository(IGitRepositoryView, IGitRepositoryModerateAttributes,
90 IGitRepositoryModerate, IGitRepositoryEditableAttributes,
91- IGitRepositoryEdit):
92+ IGitRepositoryEdit, IGitRepositoryExpensiveRequest):
93 """A Git repository."""
94
95 private = exported(Bool(
96diff --git a/lib/lp/code/model/githosting.py b/lib/lp/code/model/githosting.py
97index 74f4ec0..ea0f893 100644
98--- a/lib/lp/code/model/githosting.py
99+++ b/lib/lp/code/model/githosting.py
100@@ -27,13 +27,13 @@ from six.moves.urllib.parse import (
101 from zope.interface import implementer
102
103 from lp.code.errors import (
104+ CannotRepackRepository,
105 GitReferenceDeletionFault,
106 GitRepositoryBlobNotFound,
107 GitRepositoryCreationFault,
108 GitRepositoryDeletionFault,
109 GitRepositoryScanFault,
110 GitTargetError,
111- NoSuchGitReference,
112 )
113 from lp.code.interfaces.githosting import IGitHostingClient
114 from lp.services.config import config
115@@ -309,3 +309,18 @@ class GitHostingClient:
116 raise GitReferenceDeletionFault(
117 "Error deleting %s from repo %s: HTTP %s" %
118 (ref, path, e.response.status_code))
119+
120+ def repackRepository(self, path, logger=None):
121+ """See `IGitHostingClient`."""
122+
123+ url = "/repo/%s/repack" % path
124+ try:
125+ if logger is not None:
126+ logger.info(
127+ "Repacking repository %s" % (
128+ path))
129+ return self._post(url)
130+ except requests.RequestException as e:
131+ raise CannotRepackRepository(
132+ "Failed to repack Git repository %s: %s" %
133+ (path, six.text_type(e)))
134diff --git a/lib/lp/code/model/gitrepository.py b/lib/lp/code/model/gitrepository.py
135index fb5e703..7edd906 100644
136--- a/lib/lp/code/model/gitrepository.py
137+++ b/lib/lp/code/model/gitrepository.py
138@@ -469,6 +469,9 @@ class GitRepository(StormBase, WebhookTargetMixin, GitIdentityMixin):
139 namespace.moveRepository(self, user, rename_if_necessary=True)
140 self._reconcileAccess()
141
142+ def repackRepository(self):
143+ getUtility(IGitHostingClient).repackRepository(self.getInternalPath())
144+
145 @property
146 def namespace(self):
147 """See `IGitRepository`."""
148diff --git a/lib/lp/code/model/tests/test_githosting.py b/lib/lp/code/model/tests/test_githosting.py
149index 7011bbe..16a2488 100644
150--- a/lib/lp/code/model/tests/test_githosting.py
151+++ b/lib/lp/code/model/tests/test_githosting.py
152@@ -38,6 +38,7 @@ from zope.interface import implementer
153 from zope.security.proxy import removeSecurityProxy
154
155 from lp.code.errors import (
156+ CannotRepackRepository,
157 GitReferenceDeletionFault,
158 GitRepositoryBlobNotFound,
159 GitRepositoryCreationFault,
160@@ -484,3 +485,16 @@ class TestGitHostingClient(TestCase):
161 JobRunner([job]).runAll()
162 self.assertEqual(JobStatus.COMPLETED, job.job.status)
163 self.assertEqual({"refs/heads/master": {}}, job.refs)
164+
165+ def test_repack(self):
166+ with self.mockRequests("POST", status=200):
167+ repack = self.client.repackRepository("/repo/123")
168+ self.assertEqual(None, repack)
169+
170+ def test_repack_failure(self):
171+ with self.mockRequests("POST", status=400):
172+ self.assertRaisesWithContent(
173+ CannotRepackRepository,
174+ "Failed to repack Git repository /repo/123: "
175+ "400 Client Error: Bad Request",
176+ self.client.repackRepository, "/repo/123")
177diff --git a/lib/lp/code/model/tests/test_gitrepository.py b/lib/lp/code/model/tests/test_gitrepository.py
178index 1e12199..eb5f819 100644
179--- a/lib/lp/code/model/tests/test_gitrepository.py
180+++ b/lib/lp/code/model/tests/test_gitrepository.py
181@@ -3824,6 +3824,50 @@ class TestGitRepositoryWebservice(TestCaseWithFactory):
182
183 layer = DatabaseFunctionalLayer
184
185+ def test_repackRepository_owner(self):
186+ # Repository owner cannot repack
187+ hosting_fixture = self.useFixture(GitHostingFixture())
188+ owner_db = self.factory.makePerson()
189+ repository_db = self.factory.makeGitRepository(
190+ owner=owner_db, name="repository")
191+
192+ with person_logged_in(owner_db):
193+ self.assertRaises(
194+ Unauthorized, getattr, repository_db, 'repackRepository')
195+ self.assertEqual(0, hosting_fixture.repackRepository.call_count)
196+
197+ def test_repackRepository_admin(self):
198+ # Admins can trigger a repack
199+ hosting_fixture = self.useFixture(GitHostingFixture())
200+ owner_db = self.factory.makePerson()
201+ repository_db = self.factory.makeGitRepository(
202+ owner=owner_db, name="repository")
203+ admin = getUtility(ILaunchpadCelebrities).admin.teamowner
204+ with person_logged_in(admin):
205+ repository_db.repackRepository()
206+ self.assertEqual(
207+ [((repository_db.getInternalPath(),), {})],
208+ hosting_fixture.repackRepository.calls)
209+ self.assertEqual(1, hosting_fixture.repackRepository.call_count)
210+
211+ def test_repackRepository_registry_expert(self):
212+ # Registry experts can trigger a repack
213+ hosting_fixture = self.useFixture(GitHostingFixture())
214+ admin = getUtility(ILaunchpadCelebrities).admin.teamowner
215+ person = self.factory.makePerson()
216+ owner_db = self.factory.makePerson()
217+ repository_db = self.factory.makeGitRepository(
218+ owner=owner_db, name="repository")
219+ with admin_logged_in():
220+ getUtility(ILaunchpadCelebrities).registry_experts.addMember(
221+ person, admin)
222+ with person_logged_in(person):
223+ repository_db.repackRepository()
224+ self.assertEqual(
225+ [((repository_db.getInternalPath(),), {})],
226+ hosting_fixture.repackRepository.calls)
227+ self.assertEqual(1, hosting_fixture.repackRepository.call_count)
228+
229 def test_urls(self):
230 owner_db = self.factory.makePerson(name="person")
231 project_db = self.factory.makeProduct(name="project")
232diff --git a/lib/lp/code/tests/helpers.py b/lib/lp/code/tests/helpers.py
233index fae251a..a336a5c 100644
234--- a/lib/lp/code/tests/helpers.py
235+++ b/lib/lp/code/tests/helpers.py
236@@ -329,6 +329,7 @@ class GitHostingFixture(fixtures.Fixture):
237 self.getBlob = FakeMethod(result=blob)
238 self.delete = FakeMethod()
239 self.disable_memcache = disable_memcache
240+ self.repackRepository = FakeMethod()
241
242 def _setUp(self):
243 self.useFixture(ZopeUtilityFixture(self, IGitHostingClient))
244diff --git a/lib/lp/security.py b/lib/lp/security.py
245index 459ab8c..35c8b8c 100644
246--- a/lib/lp/security.py
247+++ b/lib/lp/security.py
248@@ -1805,6 +1805,16 @@ class DownloadFullSourcePackageTranslations(OnlyRosettaExpertsAndAdmins):
249 user.inTeam(translation_group.owner)))
250
251
252+class GitRepositoryExpensiveRequest(AuthorizationBase):
253+ """Restrict git repository repacks."""
254+
255+ permission = 'launchpad.ExpensiveRequest'
256+ usedfor = IGitRepository
257+
258+ def checkAuthenticated(self, user):
259+ return user.in_registry_experts or user.in_admin
260+
261+
262 class EditProductRelease(EditByOwnersOrAdmins):
263 permission = 'launchpad.Edit'
264 usedfor = IProductRelease

Subscribers

People subscribed via source and target branches

to status/vote changes: