Merge ~cjwatson/launchpad:move-package into launchpad:master
- Git
- lp:~cjwatson/launchpad
- move-package
- Merge into master
Proposed by
Colin Watson
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Colin Watson | ||||
Approved revision: | 0ed27c6fcfba459ec61213fb6fd6a7a6b33c2c1a | ||||
Merge reported by: | Otto Co-Pilot | ||||
Merged at revision: | not available | ||||
Proposed branch: | ~cjwatson/launchpad:move-package | ||||
Merge into: | launchpad:master | ||||
Diff against target: |
808 lines (+312/-50) 11 files modified
lib/lp/code/vocabularies/sourcepackagerecipe.py (+2/-2) lib/lp/soyuz/browser/archive.py (+2/-11) lib/lp/soyuz/interfaces/archive.py (+3/-1) lib/lp/soyuz/interfaces/packagecopyjob.py (+7/-2) lib/lp/soyuz/model/archive.py (+9/-6) lib/lp/soyuz/model/packagecopyjob.py (+15/-9) lib/lp/soyuz/scripts/packagecopier.py (+63/-9) lib/lp/soyuz/scripts/tests/test_copypackage.py (+44/-0) lib/lp/soyuz/tests/test_archive.py (+124/-5) lib/lp/soyuz/tests/test_packagecopyjob.py (+28/-3) lib/lp/soyuz/vocabularies.py (+15/-2) |
||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Kristian Glass (community) | Approve | ||
Review via email: mp+373942@code.launchpad.net |
Commit message
Add an atomic "move package" operation
Archive.copyPackage and Archive.
argument, which causes the source publication to be deleted if the copy
succeeds.
This allows us to fix a long-standing problem with Ubuntu's
proposed-migration process: it needs to do a copy and delete when
migrating packages from devel-proposed to devel, but since the copy is
asynchronous it can fail without proposed-migration being aware of this,
leading to the package in question simply being removed. Moving the
deletion into the copier avoids this problem.
LP: #1329052
Description of the change
To post a comment you must log in.
Revision history for this message
Otto Co-Pilot (otto-copilot) wrote : | # |
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | diff --git a/lib/lp/code/vocabularies/sourcepackagerecipe.py b/lib/lp/code/vocabularies/sourcepackagerecipe.py |
2 | index 0c56101..31cf4bc 100644 |
3 | --- a/lib/lp/code/vocabularies/sourcepackagerecipe.py |
4 | +++ b/lib/lp/code/vocabularies/sourcepackagerecipe.py |
5 | @@ -1,4 +1,4 @@ |
6 | -# Copyright 2010-2018 Canonical Ltd. This software is licensed under the |
7 | +# Copyright 2010-2019 Canonical Ltd. This software is licensed under the |
8 | # GNU Affero General Public License version 3 (see the file LICENSE). |
9 | |
10 | """Source Package Recipe vocabularies used in the lp/code modules.""" |
11 | @@ -22,8 +22,8 @@ from lp.services.webapp.vocabulary import ( |
12 | IHugeVocabulary, |
13 | SQLObjectVocabularyBase, |
14 | ) |
15 | -from lp.soyuz.browser.archive import make_archive_vocabulary |
16 | from lp.soyuz.interfaces.archive import IArchiveSet |
17 | +from lp.soyuz.vocabularies import make_archive_vocabulary |
18 | |
19 | |
20 | @implementer(IHugeVocabulary) |
21 | diff --git a/lib/lp/soyuz/browser/archive.py b/lib/lp/soyuz/browser/archive.py |
22 | index 6dd6cfe..3170ea0 100644 |
23 | --- a/lib/lp/soyuz/browser/archive.py |
24 | +++ b/lib/lp/soyuz/browser/archive.py |
25 | @@ -1,4 +1,4 @@ |
26 | -# Copyright 2009-2018 Canonical Ltd. This software is licensed under the |
27 | +# Copyright 2009-2019 Canonical Ltd. This software is licensed under the |
28 | # GNU Affero General Public License version 3 (see the file LICENSE). |
29 | |
30 | """Browser views for archive.""" |
31 | @@ -23,7 +23,6 @@ __all__ = [ |
32 | 'ArchiveView', |
33 | 'ArchiveViewBase', |
34 | 'EnableProcessorsMixin', |
35 | - 'make_archive_vocabulary', |
36 | 'PackageCopyingMixin', |
37 | 'traverse_named_ppa', |
38 | ] |
39 | @@ -175,6 +174,7 @@ from lp.soyuz.model.archive import ( |
40 | ) |
41 | from lp.soyuz.model.publishing import SourcePackagePublishingHistory |
42 | from lp.soyuz.scripts.packagecopier import check_copy_permissions |
43 | +from lp.soyuz.vocabularies import make_archive_vocabulary |
44 | |
45 | |
46 | class ArchiveBadges(HasBadgeBase): |
47 | @@ -1433,15 +1433,6 @@ class PackageCopyingMixin: |
48 | return True |
49 | |
50 | |
51 | -def make_archive_vocabulary(archives): |
52 | - terms = [] |
53 | - for archive in archives: |
54 | - label = '%s [%s]' % (archive.displayname, archive.reference) |
55 | - terms.append(SimpleTerm(archive, archive.reference, label)) |
56 | - terms.sort(key=lambda x: x.value.reference) |
57 | - return SimpleVocabulary(terms) |
58 | - |
59 | - |
60 | class ArchivePackageCopyingView(ArchiveSourceSelectionFormView, |
61 | PackageCopyingMixin): |
62 | """Archive package copying view class. |
63 | diff --git a/lib/lp/soyuz/interfaces/archive.py b/lib/lp/soyuz/interfaces/archive.py |
64 | index e28f08b..2aae255 100644 |
65 | --- a/lib/lp/soyuz/interfaces/archive.py |
66 | +++ b/lib/lp/soyuz/interfaces/archive.py |
67 | @@ -1522,7 +1522,7 @@ class IArchiveView(IHasBuildRecords): |
68 | person, to_series=None, include_binaries=False, |
69 | sponsored=None, unembargo=False, auto_approve=False, |
70 | silent=False, from_pocket=None, from_series=None, |
71 | - phased_update_percentage=None): |
72 | + phased_update_percentage=None, move=False): |
73 | """Copy a single named source into this archive. |
74 | |
75 | Asynchronously copy a specific version of a named source to the |
76 | @@ -1563,6 +1563,8 @@ class IArchiveView(IHasBuildRecords): |
77 | omitted, copy from any series with a matching version. |
78 | :param phased_update_percentage: the phased update percentage to |
79 | apply to the copied publication. |
80 | + :param move: if True, delete the source publication after copying it |
81 | + to the destination. |
82 | |
83 | :raises NoSuchSourcePackageName: if the source name is invalid |
84 | :raises PocketNotFound: if the pocket name is invalid |
85 | diff --git a/lib/lp/soyuz/interfaces/packagecopyjob.py b/lib/lp/soyuz/interfaces/packagecopyjob.py |
86 | index 32ef2dc..043fff5 100644 |
87 | --- a/lib/lp/soyuz/interfaces/packagecopyjob.py |
88 | +++ b/lib/lp/soyuz/interfaces/packagecopyjob.py |
89 | @@ -1,4 +1,4 @@ |
90 | -# Copyright 2010-2013 Canonical Ltd. This software is licensed under the |
91 | +# Copyright 2010-2019 Canonical Ltd. This software is licensed under the |
92 | # GNU Affero General Public License version 3 (see the file LICENSE). |
93 | |
94 | __metaclass__ = type |
95 | @@ -130,7 +130,7 @@ class IPlainPackageCopyJobSource(IJobSource): |
96 | copy_policy=PackageCopyPolicy.INSECURE, requester=None, |
97 | sponsored=None, unembargo=False, auto_approve=False, |
98 | silent=False, source_distroseries=None, source_pocket=None, |
99 | - phased_update_percentage=None): |
100 | + phased_update_percentage=None, move=False): |
101 | """Create a new `IPlainPackageCopyJob`. |
102 | |
103 | :param package_name: The name of the source package to copy. |
104 | @@ -162,6 +162,8 @@ class IPlainPackageCopyJobSource(IJobSource): |
105 | from any pocket with a matching version. |
106 | :param phased_update_percentage: The phased update percentage to |
107 | apply to the copied publication. |
108 | + :param move: If True, delete the source publication after copying it |
109 | + to the destination. |
110 | """ |
111 | |
112 | def createMultiple(target_distroseries, copy_tasks, requester, |
113 | @@ -254,6 +256,9 @@ class IPlainPackageCopyJob(IRunnableJob): |
114 | phased_update_percentage = Int( |
115 | title=_("Phased update percentage"), required=False, readonly=True) |
116 | |
117 | + move = Bool( |
118 | + title=_("Delete source after copy"), required=False, readonly=True) |
119 | + |
120 | def addSourceOverride(override): |
121 | """Add an `ISourceOverride` to the metadata.""" |
122 | |
123 | diff --git a/lib/lp/soyuz/model/archive.py b/lib/lp/soyuz/model/archive.py |
124 | index acf8541..cb3f089 100644 |
125 | --- a/lib/lp/soyuz/model/archive.py |
126 | +++ b/lib/lp/soyuz/model/archive.py |
127 | @@ -1829,7 +1829,7 @@ class Archive(SQLBase): |
128 | person, to_series=None, include_binaries=False, |
129 | sponsored=None, unembargo=False, auto_approve=False, |
130 | silent=False, from_pocket=None, from_series=None, |
131 | - phased_update_percentage=None): |
132 | + phased_update_percentage=None, move=False): |
133 | """See `IArchive`.""" |
134 | # Asynchronously copy a package using the job system. |
135 | from lp.soyuz.scripts.packagecopier import check_copy_permissions |
136 | @@ -1854,7 +1854,8 @@ class Archive(SQLBase): |
137 | from_pocket=from_pocket) |
138 | if series is None: |
139 | series = source.distroseries |
140 | - check_copy_permissions(person, self, series, pocket, [source]) |
141 | + check_copy_permissions( |
142 | + person, self, series, pocket, [source], move=move) |
143 | |
144 | job_source = getUtility(IPlainPackageCopyJobSource) |
145 | job_source.create( |
146 | @@ -1866,12 +1867,12 @@ class Archive(SQLBase): |
147 | sponsored=sponsored, unembargo=unembargo, |
148 | auto_approve=auto_approve, silent=silent, |
149 | source_distroseries=from_series, source_pocket=from_pocket, |
150 | - phased_update_percentage=phased_update_percentage) |
151 | + phased_update_percentage=phased_update_percentage, move=move) |
152 | |
153 | def copyPackages(self, source_names, from_archive, to_pocket, |
154 | person, to_series=None, from_series=None, |
155 | include_binaries=None, sponsored=None, unembargo=False, |
156 | - auto_approve=False, silent=False): |
157 | + auto_approve=False, silent=False, move=False): |
158 | """See `IArchive`.""" |
159 | from lp.soyuz.scripts.packagecopier import check_copy_permissions |
160 | sources = self._collectLatestPublishedSources( |
161 | @@ -1880,7 +1881,8 @@ class Archive(SQLBase): |
162 | # Now do a mass check of permissions. |
163 | pocket = self._text_to_pocket(to_pocket) |
164 | series = self._text_to_series(to_series) |
165 | - check_copy_permissions(person, self, series, pocket, sources) |
166 | + check_copy_permissions( |
167 | + person, self, series, pocket, sources, move=move) |
168 | |
169 | # If we get this far then we can create the PackageCopyJob. |
170 | copy_tasks = [] |
171 | @@ -1899,7 +1901,8 @@ class Archive(SQLBase): |
172 | job_source.createMultiple( |
173 | copy_tasks, person, copy_policy=PackageCopyPolicy.MASS_SYNC, |
174 | include_binaries=include_binaries, sponsored=sponsored, |
175 | - unembargo=unembargo, auto_approve=auto_approve, silent=silent) |
176 | + unembargo=unembargo, auto_approve=auto_approve, silent=silent, |
177 | + move=move) |
178 | |
179 | def _collectLatestPublishedSources(self, from_archive, from_series, |
180 | source_names): |
181 | diff --git a/lib/lp/soyuz/model/packagecopyjob.py b/lib/lp/soyuz/model/packagecopyjob.py |
182 | index b4d92b2..90389e9 100644 |
183 | --- a/lib/lp/soyuz/model/packagecopyjob.py |
184 | +++ b/lib/lp/soyuz/model/packagecopyjob.py |
185 | @@ -1,4 +1,4 @@ |
186 | -# Copyright 2010-2016 Canonical Ltd. This software is licensed under the |
187 | +# Copyright 2010-2019 Canonical Ltd. This software is licensed under the |
188 | # GNU Affero General Public License version 3 (see the file LICENSE). |
189 | |
190 | __metaclass__ = type |
191 | @@ -275,7 +275,7 @@ class PlainPackageCopyJob(PackageCopyJobDerived): |
192 | include_binaries, sponsored=None, unembargo=False, |
193 | auto_approve=False, silent=False, |
194 | source_distroseries=None, source_pocket=None, |
195 | - phased_update_percentage=None): |
196 | + phased_update_percentage=None, move=False): |
197 | """Produce a metadata dict for this job.""" |
198 | return { |
199 | 'target_pocket': target_pocket.value, |
200 | @@ -289,6 +289,7 @@ class PlainPackageCopyJob(PackageCopyJobDerived): |
201 | source_distroseries.name if source_distroseries else None, |
202 | 'source_pocket': source_pocket.value if source_pocket else None, |
203 | 'phased_update_percentage': phased_update_percentage, |
204 | + 'move': move, |
205 | } |
206 | |
207 | @classmethod |
208 | @@ -298,14 +299,14 @@ class PlainPackageCopyJob(PackageCopyJobDerived): |
209 | copy_policy=PackageCopyPolicy.INSECURE, requester=None, |
210 | sponsored=None, unembargo=False, auto_approve=False, |
211 | silent=False, source_distroseries=None, source_pocket=None, |
212 | - phased_update_percentage=None): |
213 | + phased_update_percentage=None, move=False): |
214 | """See `IPlainPackageCopyJobSource`.""" |
215 | assert package_version is not None, "No package version specified." |
216 | assert requester is not None, "No requester specified." |
217 | metadata = cls._makeMetadata( |
218 | target_pocket, package_version, include_binaries, sponsored, |
219 | unembargo, auto_approve, silent, source_distroseries, |
220 | - source_pocket, phased_update_percentage) |
221 | + source_pocket, phased_update_percentage, move) |
222 | job = PackageCopyJob( |
223 | job_type=cls.class_job_type, |
224 | source_archive=source_archive, |
225 | @@ -323,7 +324,7 @@ class PlainPackageCopyJob(PackageCopyJobDerived): |
226 | @classmethod |
227 | def _composeJobInsertionTuple(cls, copy_policy, include_binaries, job_id, |
228 | copy_task, sponsored, unembargo, |
229 | - auto_approve, silent): |
230 | + auto_approve, silent, move): |
231 | """Create an SQL fragment for inserting a job into the database. |
232 | |
233 | :return: A string representing an SQL tuple containing initializers |
234 | @@ -340,7 +341,7 @@ class PlainPackageCopyJob(PackageCopyJobDerived): |
235 | ) = copy_task |
236 | metadata = cls._makeMetadata( |
237 | target_pocket, package_version, include_binaries, sponsored, |
238 | - unembargo, auto_approve, silent) |
239 | + unembargo, auto_approve, silent, move=move) |
240 | data = ( |
241 | cls.class_job_type, target_distroseries, copy_policy, |
242 | source_archive, target_archive, package_name, job_id, |
243 | @@ -351,14 +352,15 @@ class PlainPackageCopyJob(PackageCopyJobDerived): |
244 | def createMultiple(cls, copy_tasks, requester, |
245 | copy_policy=PackageCopyPolicy.INSECURE, |
246 | include_binaries=False, sponsored=None, |
247 | - unembargo=False, auto_approve=False, silent=False): |
248 | + unembargo=False, auto_approve=False, silent=False, |
249 | + move=False): |
250 | """See `IPlainPackageCopyJobSource`.""" |
251 | store = IMasterStore(Job) |
252 | job_ids = Job.createMultiple(store, len(copy_tasks), requester) |
253 | job_contents = [ |
254 | cls._composeJobInsertionTuple( |
255 | copy_policy, include_binaries, job_id, task, sponsored, |
256 | - unembargo, auto_approve, silent) |
257 | + unembargo, auto_approve, silent, move) |
258 | for job_id, task in zip(job_ids, copy_tasks)] |
259 | return bulk.create( |
260 | (PackageCopyJob.job_type, PackageCopyJob.target_distroseries, |
261 | @@ -467,6 +469,10 @@ class PlainPackageCopyJob(PackageCopyJobDerived): |
262 | return self.metadata.get('phased_update_percentage') |
263 | |
264 | @property |
265 | + def move(self): |
266 | + return self.metadata.get('move', False) |
267 | + |
268 | + @property |
269 | def requester_can_admin_target(self): |
270 | return self.target_archive.canAdministerQueue( |
271 | self.requester, self.getSourceOverride().component, |
272 | @@ -659,7 +665,7 @@ class PlainPackageCopyJob(PackageCopyJobDerived): |
273 | sponsored=self.sponsored, packageupload=pu, |
274 | unembargo=self.unembargo, |
275 | phased_update_percentage=self.phased_update_percentage, |
276 | - logger=self.logger) |
277 | + move=self.move, logger=self.logger) |
278 | |
279 | # Add a PackageDiff for this new upload if it has ancestry. |
280 | if copied_publications and not ancestry.is_empty(): |
281 | diff --git a/lib/lp/soyuz/scripts/packagecopier.py b/lib/lp/soyuz/scripts/packagecopier.py |
282 | index 3891af5..2e4a767 100644 |
283 | --- a/lib/lp/soyuz/scripts/packagecopier.py |
284 | +++ b/lib/lp/soyuz/scripts/packagecopier.py |
285 | @@ -1,4 +1,4 @@ |
286 | -# Copyright 2009-2015 Canonical Ltd. This software is licensed under the |
287 | +# Copyright 2009-2019 Canonical Ltd. This software is licensed under the |
288 | # GNU Affero General Public License version 3 (see the file LICENSE). |
289 | |
290 | """Package copying utilities.""" |
291 | @@ -15,9 +15,15 @@ __all__ = [ |
292 | |
293 | import apt_pkg |
294 | from lazr.delegates import delegate_to |
295 | -from zope.component import getUtility |
296 | +from zope.component import ( |
297 | + getAdapter, |
298 | + getUtility, |
299 | + ) |
300 | from zope.security.proxy import removeSecurityProxy |
301 | |
302 | +from lp.app.interfaces.security import IAuthorization |
303 | +from lp.registry.interfaces.role import IPersonRoles |
304 | +from lp.registry.model.person import Person |
305 | from lp.services.database.bulk import load_related |
306 | from lp.soyuz.adapters.overrides import SourceOverride |
307 | from lp.soyuz.enums import SourcePackageFormat |
308 | @@ -141,7 +147,8 @@ class CheckedCopy: |
309 | return {'status': BuildSetStatus.NEEDSBUILD} |
310 | |
311 | |
312 | -def check_copy_permissions(person, archive, series, pocket, sources): |
313 | +def check_copy_permissions(person, archive, series, pocket, sources, |
314 | + move=False): |
315 | """Check that `person` has permission to copy a package. |
316 | |
317 | :param person: User attempting the upload. |
318 | @@ -150,9 +157,12 @@ def check_copy_permissions(person, archive, series, pocket, sources): |
319 | :param pocket: Destination `Pocket`. |
320 | :param sources: Sequence of `SourcePackagePublishingHistory`s for the |
321 | packages to be copied. |
322 | + :param move: If True, also check whether `person` has permission to |
323 | + delete the sources. |
324 | :raises CannotCopy: If the copy is not allowed. |
325 | """ |
326 | # Circular import. |
327 | + from lp.soyuz.model.archive import Archive |
328 | from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease |
329 | |
330 | if person is None: |
331 | @@ -161,6 +171,12 @@ def check_copy_permissions(person, archive, series, pocket, sources): |
332 | if len(sources) > 1: |
333 | # Bulk-load the data we'll need from each source publication. |
334 | load_related(SourcePackageRelease, sources, ["sourcepackagereleaseID"]) |
335 | + if move: |
336 | + # Bulk-load at least some of the data we'll need for permission |
337 | + # checks on each source archive. Not all of this is currently |
338 | + # preloadable. |
339 | + archives = load_related(Archive, sources, ["archiveID"]) |
340 | + load_related(Person, archives, ["ownerID"]) |
341 | |
342 | # If there is a requester, check that they have upload permission into |
343 | # the destination (archive, component, pocket). This check is done |
344 | @@ -191,6 +207,29 @@ def check_copy_permissions(person, archive, series, pocket, sources): |
345 | person, override.component, pocket, dest_series): |
346 | raise CannotCopy(reason) |
347 | |
348 | + if move: |
349 | + roles = IPersonRoles(person) |
350 | + for source_archive in set(source.archive for source in sources): |
351 | + # XXX cjwatson 2019-10-09: Checking the archive rather than the |
352 | + # SPPH duplicates security adapter logic, which is unfortunate; |
353 | + # but too much of the logic required to use |
354 | + # DelegatedAuthorization-based adapters such as the one used for |
355 | + # launchpad.Edit on SPPH lives in |
356 | + # lp.services.webapp.authorization and is hard to use without a |
357 | + # full Zope interaction. |
358 | + source_archive_authz = getAdapter( |
359 | + source_archive, IAuthorization, "launchpad.Append") |
360 | + if not source_archive_authz.checkAuthenticated(roles): |
361 | + raise CannotCopy( |
362 | + "%s is not permitted to delete publications from %s." % |
363 | + (person.display_name, source_archive.displayname)) |
364 | + for source in sources: |
365 | + if not source.archive.canModifySuite( |
366 | + source.distroseries, source.pocket): |
367 | + raise CannotCopy( |
368 | + "Cannot delete publications from suite '%s'" % |
369 | + source.distroseries.getSuite(source.pocket)) |
370 | + |
371 | |
372 | class CopyChecker: |
373 | """Check copy candiates. |
374 | @@ -388,7 +427,7 @@ class CopyChecker: |
375 | "different contents." % lf.libraryfile.filename) |
376 | |
377 | def checkCopy(self, source, series, pocket, person=None, |
378 | - check_permissions=True): |
379 | + check_permissions=True, move=False): |
380 | """Check if the source can be copied to the given location. |
381 | |
382 | Check possible conflicting publications in the destination archive. |
383 | @@ -407,13 +446,15 @@ class CopyChecker: |
384 | :param person: requester `IPerson`. |
385 | :param check_permissions: boolean indicating whether or not the |
386 | requester's permissions to copy should be checked. |
387 | + :param move: if True, also check whether `person` has permission to |
388 | + delete the source. |
389 | |
390 | :raise CannotCopy when a copy is not allowed to be performed |
391 | containing the reason of the error. |
392 | """ |
393 | if check_permissions: |
394 | check_copy_permissions( |
395 | - person, self.archive, series, pocket, [source]) |
396 | + person, self.archive, series, pocket, [source], move=move) |
397 | |
398 | if series.distribution != self.archive.distribution: |
399 | raise CannotCopy( |
400 | @@ -483,7 +524,7 @@ def do_copy(sources, archive, series, pocket, include_binaries=False, |
401 | send_email=False, strict_binaries=True, close_bugs=True, |
402 | create_dsd_job=True, announce_from_person=None, sponsored=None, |
403 | packageupload=None, unembargo=False, phased_update_percentage=None, |
404 | - logger=None): |
405 | + move=False, logger=None): |
406 | """Perform the complete copy of the given sources incrementally. |
407 | |
408 | Verifies if each copy can be performed using `CopyChecker` and |
409 | @@ -532,6 +573,8 @@ def do_copy(sources, archive, series, pocket, include_binaries=False, |
410 | doing so. |
411 | :param phased_update_percentage: The phased update percentage to apply |
412 | to the copied publication. |
413 | + :param move: If True, delete the source publication after copying to the |
414 | + destination. |
415 | :param logger: An optional logger. |
416 | |
417 | :raise CannotCopy when one or more copies were not allowed. The error |
418 | @@ -554,7 +597,8 @@ def do_copy(sources, archive, series, pocket, include_binaries=False, |
419 | destination_series = series |
420 | try: |
421 | copy_checker.checkCopy( |
422 | - source, destination_series, pocket, person, check_permissions) |
423 | + source, destination_series, pocket, person, check_permissions, |
424 | + move=move) |
425 | except CannotCopy as reason: |
426 | errors.append("%s (%s)" % (source.displayname, reason)) |
427 | continue |
428 | @@ -610,7 +654,8 @@ def do_copy(sources, archive, series, pocket, include_binaries=False, |
429 | override, close_bugs=close_bugs, create_dsd_job=create_dsd_job, |
430 | close_bugs_since_version=old_version, creator=creator, |
431 | sponsor=sponsor, packageupload=packageupload, |
432 | - phased_update_percentage=phased_update_percentage, logger=logger) |
433 | + phased_update_percentage=phased_update_percentage, move=move, |
434 | + logger=logger) |
435 | if send_email and sub_copies: |
436 | mailer = PackageUploadMailer.forAction( |
437 | 'accepted', person, source.sourcepackagerelease, [], [], |
438 | @@ -639,7 +684,7 @@ def _do_direct_copy(source, archive, series, pocket, include_binaries, |
439 | override=None, close_bugs=True, create_dsd_job=True, |
440 | close_bugs_since_version=None, creator=None, |
441 | sponsor=None, packageupload=None, |
442 | - phased_update_percentage=None, logger=None): |
443 | + phased_update_percentage=None, move=False, logger=None): |
444 | """Copy publishing records to another location. |
445 | |
446 | Copy each item of the given list of `SourcePackagePublishingHistory` |
447 | @@ -671,6 +716,8 @@ def _do_direct_copy(source, archive, series, pocket, include_binaries, |
448 | to be created. |
449 | :param phased_update_percentage: The phased update percentage to apply |
450 | to the copied publication. |
451 | + :param move: If True, delete the source publication after copying to the |
452 | + destination. |
453 | :param logger: An optional logger. |
454 | |
455 | :return: a list of `ISourcePackagePublishingHistory` and |
456 | @@ -742,4 +789,11 @@ def _do_direct_copy(source, archive, series, pocket, include_binaries, |
457 | # XXX cjwatson 2012-06-22 bug=869308: Fails to honour P-a-s. |
458 | source_copy.createMissingBuilds(logger=logger) |
459 | |
460 | + if move: |
461 | + removal_comment = "Moved to %s" % series.getSuite(pocket) |
462 | + if archive != source.archive: |
463 | + removal_comment += " in %s" % archive.reference |
464 | + getUtility(IPublishingSet).requestDeletion( |
465 | + [source], creator, removal_comment=removal_comment) |
466 | + |
467 | return copies |
468 | diff --git a/lib/lp/soyuz/scripts/tests/test_copypackage.py b/lib/lp/soyuz/scripts/tests/test_copypackage.py |
469 | index 388fcf7..eee530a 100644 |
470 | --- a/lib/lp/soyuz/scripts/tests/test_copypackage.py |
471 | +++ b/lib/lp/soyuz/scripts/tests/test_copypackage.py |
472 | @@ -1732,6 +1732,50 @@ class TestDoDirectCopy(BaseDoCopyTests, TestCaseWithFactory): |
473 | self.assertIsInstance(copies[1], BinaryPackagePublishingHistory) |
474 | self.assertEqual("i386", copies[1].distroarchseries.architecturetag) |
475 | |
476 | + def test_copy_without_move(self): |
477 | + # A copy with move=False (the default) leaves the source publication |
478 | + # intact. |
479 | + nobby, archive, source = self._setup_archive() |
480 | + target_archive = self.factory.makeArchive( |
481 | + distribution=self.test_publisher.ubuntutest) |
482 | + [copied_source] = do_copy( |
483 | + [source], target_archive, nobby, source.pocket, |
484 | + include_binaries=False, person=target_archive.owner, |
485 | + check_permissions=False, send_email=False) |
486 | + self.assertEqual(PackagePublishingStatus.PENDING, copied_source.status) |
487 | + self.assertEqual(PackagePublishingStatus.PENDING, source.status) |
488 | + |
489 | + def test_copy_with_move(self): |
490 | + # A copy with move=True deletes the source publication. |
491 | + nobby, archive, source = self._setup_archive() |
492 | + target_archive = self.factory.makeArchive( |
493 | + distribution=self.test_publisher.ubuntutest) |
494 | + [copied_source] = do_copy( |
495 | + [source], target_archive, nobby, source.pocket, |
496 | + include_binaries=False, person=target_archive.owner, |
497 | + check_permissions=False, send_email=False, move=True) |
498 | + self.assertEqual(PackagePublishingStatus.PENDING, copied_source.status) |
499 | + self.assertEqual(PackagePublishingStatus.DELETED, source.status) |
500 | + self.assertEqual( |
501 | + "Moved to %s in %s" % ( |
502 | + nobby.getSuite(source.pocket), target_archive.reference), |
503 | + source.removal_comment) |
504 | + |
505 | + def test_copy_with_move_failure(self): |
506 | + # If a copy with move=True fails, then the source publication is |
507 | + # left intact. |
508 | + nobby, archive, source = self._setup_archive() |
509 | + self.test_publisher.getPubSource( |
510 | + sourcename=source.source_package_name, |
511 | + archive=nobby.main_archive, version="1.0-2", |
512 | + architecturehintlist="any") |
513 | + self.assertRaises( |
514 | + CannotCopy, do_copy, |
515 | + [source], archive, nobby, source.pocket, |
516 | + include_binaries=False, person=source.sourcepackagerelease.creator, |
517 | + check_permissions=False, send_email=False, move=True) |
518 | + self.assertEqual(PackagePublishingStatus.PENDING, source.status) |
519 | + |
520 | |
521 | class TestCopyBuildRecords(TestCaseWithFactory): |
522 | """Test handling of binaries and their build records when copying.""" |
523 | diff --git a/lib/lp/soyuz/tests/test_archive.py b/lib/lp/soyuz/tests/test_archive.py |
524 | index 09f0141..b015953 100644 |
525 | --- a/lib/lp/soyuz/tests/test_archive.py |
526 | +++ b/lib/lp/soyuz/tests/test_archive.py |
527 | @@ -2798,20 +2798,23 @@ class TestCopyPackage(TestCaseWithFactory): |
528 | |
529 | layer = DatabaseFunctionalLayer |
530 | |
531 | - def _setup_copy_data(self, source_distribution=None, source_private=False, |
532 | + def _setup_copy_data(self, source_distribution=None, source_purpose=None, |
533 | + source_private=False, source_pocket=None, |
534 | target_purpose=None, |
535 | target_status=SeriesStatus.DEVELOPMENT, |
536 | same_distribution=False): |
537 | if target_purpose is None: |
538 | target_purpose = ArchivePurpose.PPA |
539 | source_archive = self.factory.makeArchive( |
540 | - distribution=source_distribution, private=source_private) |
541 | + distribution=source_distribution, purpose=source_purpose, |
542 | + private=source_private) |
543 | target_distribution = ( |
544 | source_archive.distribution if same_distribution else None) |
545 | target_archive = self.factory.makeArchive( |
546 | distribution=target_distribution, purpose=target_purpose) |
547 | source = self.factory.makeSourcePackagePublishingHistory( |
548 | - archive=source_archive, status=PackagePublishingStatus.PUBLISHED) |
549 | + archive=source_archive, pocket=source_pocket, |
550 | + status=PackagePublishingStatus.PUBLISHED) |
551 | with person_logged_in(source_archive.owner): |
552 | source_name = source.source_package_name |
553 | version = source.source_package_version |
554 | @@ -2854,7 +2857,8 @@ class TestCopyPackage(TestCaseWithFactory): |
555 | include_binaries=False, |
556 | sponsored=sponsored, |
557 | copy_policy=PackageCopyPolicy.INSECURE, |
558 | - phased_update_percentage=30)) |
559 | + phased_update_percentage=30, |
560 | + move=False)) |
561 | |
562 | def test_copyPackage_disallows_non_primary_archive_uploaders(self): |
563 | # If copying to a primary archive and you're not an uploader for |
564 | @@ -3052,6 +3056,63 @@ class TestCopyPackage(TestCaseWithFactory): |
565 | self.assertEqual(source.distroseries, copy_job.source_distroseries) |
566 | self.assertEqual(source.pocket, copy_job.source_pocket) |
567 | |
568 | + def test_copyPackage_move(self): |
569 | + # Passing move=True causes copyPackage to create a copy job that |
570 | + # will delete the source publication after copying. |
571 | + (source, source_archive, source_name, target_archive, to_pocket, |
572 | + to_series, version) = self._setup_copy_data( |
573 | + source_distribution=self.factory.makeDistribution()) |
574 | + with person_logged_in(target_archive.owner): |
575 | + target_archive.newComponentUploader(source_archive.owner, "main") |
576 | + with person_logged_in(source_archive.owner): |
577 | + target_archive.copyPackage( |
578 | + source_name, version, source_archive, to_pocket.name, |
579 | + to_series=to_series.name, include_binaries=True, |
580 | + person=source_archive.owner, move=True) |
581 | + |
582 | + # There should be one copy job, with move=True set. |
583 | + job_source = getUtility(IPlainPackageCopyJobSource) |
584 | + copy_job = job_source.getActiveJobs(target_archive).one() |
585 | + self.assertTrue(copy_job.move) |
586 | + |
587 | + def test_copyPackage_move_without_permission(self): |
588 | + # Passing move=True checks that the user is permitted to delete the |
589 | + # source publication. |
590 | + (source, source_archive, source_name, target_archive, to_pocket, |
591 | + to_series, version) = self._setup_copy_data( |
592 | + source_distribution=self.factory.makeDistribution()) |
593 | + with person_logged_in(target_archive.owner): |
594 | + expected_error = ( |
595 | + "%s is not permitted to delete publications from %s." % ( |
596 | + target_archive.owner.display_name, |
597 | + source_archive.displayname)) |
598 | + self.assertRaisesWithContent( |
599 | + CannotCopy, expected_error, target_archive.copyPackage, |
600 | + source_name, version, source_archive, to_pocket.name, |
601 | + to_series=to_series.name, include_binaries=True, |
602 | + person=target_archive.owner, move=True) |
603 | + |
604 | + def test_copyPackage_move_from_immutable_suite(self): |
605 | + # Passing move=True checks that the source suite can be modified. |
606 | + (source, source_archive, source_name, target_archive, to_pocket, |
607 | + to_series, version) = self._setup_copy_data( |
608 | + source_distribution=self.factory.makeDistribution(), |
609 | + source_purpose=ArchivePurpose.PRIMARY, |
610 | + source_pocket=PackagePublishingPocket.RELEASE) |
611 | + with person_logged_in(target_archive.owner): |
612 | + target_archive.newComponentUploader(source_archive.owner, "main") |
613 | + removeSecurityProxy(source.distroseries).status = ( |
614 | + SeriesStatus.SUPPORTED) |
615 | + with person_logged_in(source_archive.owner): |
616 | + expected_error = ( |
617 | + "Cannot delete publications from suite '%s'" % ( |
618 | + source.distroseries.getSuite(source.pocket))) |
619 | + self.assertRaisesWithContent( |
620 | + CannotCopy, expected_error, target_archive.copyPackage, |
621 | + source_name, version, source_archive, to_pocket.name, |
622 | + to_series=to_series.name, include_binaries=True, |
623 | + person=source_archive.owner, move=True) |
624 | + |
625 | def test_copyPackages_with_single_package(self): |
626 | (source, source_archive, source_name, target_archive, to_pocket, |
627 | to_series, version) = self._setup_copy_data() |
628 | @@ -3080,7 +3141,8 @@ class TestCopyPackage(TestCaseWithFactory): |
629 | target_pocket=to_pocket, |
630 | include_binaries=False, |
631 | sponsored=sponsored, |
632 | - copy_policy=PackageCopyPolicy.MASS_SYNC)) |
633 | + copy_policy=PackageCopyPolicy.MASS_SYNC, |
634 | + move=False)) |
635 | |
636 | def test_copyPackages_with_multiple_packages(self): |
637 | # PENDING and PUBLISHED packages should both be copied. |
638 | @@ -3297,6 +3359,63 @@ class TestCopyPackage(TestCaseWithFactory): |
639 | copy_job = job_source.getActiveJobs(target_archive).one() |
640 | self.assertEqual(to_pocket, copy_job.target_pocket) |
641 | |
642 | + def test_copyPackages_move(self): |
643 | + # Passing move=True causes copyPackages to create copy jobs that |
644 | + # will delete the source publication after copying. |
645 | + (source, source_archive, source_name, target_archive, to_pocket, |
646 | + to_series, version) = self._setup_copy_data( |
647 | + source_distribution=self.factory.makeDistribution()) |
648 | + with person_logged_in(target_archive.owner): |
649 | + target_archive.newComponentUploader(source_archive.owner, "main") |
650 | + with person_logged_in(source_archive.owner): |
651 | + target_archive.copyPackages( |
652 | + [source_name], source_archive, to_pocket.name, |
653 | + to_series=to_series.name, include_binaries=True, |
654 | + person=source_archive.owner, move=True) |
655 | + |
656 | + # There should be one copy job, with move=True set. |
657 | + job_source = getUtility(IPlainPackageCopyJobSource) |
658 | + copy_job = job_source.getActiveJobs(target_archive).one() |
659 | + self.assertTrue(copy_job.move) |
660 | + |
661 | + def test_copyPackages_move_without_permission(self): |
662 | + # Passing move=True checks that the user is permitted to delete the |
663 | + # source publication. |
664 | + (source, source_archive, source_name, target_archive, to_pocket, |
665 | + to_series, version) = self._setup_copy_data( |
666 | + source_distribution=self.factory.makeDistribution()) |
667 | + with person_logged_in(target_archive.owner): |
668 | + expected_error = ( |
669 | + "%s is not permitted to delete publications from %s." % ( |
670 | + target_archive.owner.display_name, |
671 | + source_archive.displayname)) |
672 | + self.assertRaisesWithContent( |
673 | + CannotCopy, expected_error, target_archive.copyPackages, |
674 | + [source_name], source_archive, to_pocket.name, |
675 | + to_series=to_series.name, include_binaries=True, |
676 | + person=target_archive.owner, move=True) |
677 | + |
678 | + def test_copyPackages_move_from_immutable_suite(self): |
679 | + # Passing move=True checks that the source suite can be modified. |
680 | + (source, source_archive, source_name, target_archive, to_pocket, |
681 | + to_series, version) = self._setup_copy_data( |
682 | + source_distribution=self.factory.makeDistribution(), |
683 | + source_purpose=ArchivePurpose.PRIMARY, |
684 | + source_pocket=PackagePublishingPocket.RELEASE) |
685 | + with person_logged_in(target_archive.owner): |
686 | + target_archive.newComponentUploader(source_archive.owner, "main") |
687 | + removeSecurityProxy(source.distroseries).status = ( |
688 | + SeriesStatus.SUPPORTED) |
689 | + with person_logged_in(source_archive.owner): |
690 | + expected_error = ( |
691 | + "Cannot delete publications from suite '%s'" % ( |
692 | + source.distroseries.getSuite(source.pocket))) |
693 | + self.assertRaisesWithContent( |
694 | + CannotCopy, expected_error, target_archive.copyPackages, |
695 | + [source_name], source_archive, to_pocket.name, |
696 | + to_series=to_series.name, include_binaries=True, |
697 | + person=source_archive.owner, move=True) |
698 | + |
699 | |
700 | class TestgetAllPublishedBinaries(TestCaseWithFactory): |
701 | |
702 | diff --git a/lib/lp/soyuz/tests/test_packagecopyjob.py b/lib/lp/soyuz/tests/test_packagecopyjob.py |
703 | index 68c1f95..6817c10 100644 |
704 | --- a/lib/lp/soyuz/tests/test_packagecopyjob.py |
705 | +++ b/lib/lp/soyuz/tests/test_packagecopyjob.py |
706 | @@ -1,4 +1,4 @@ |
707 | -# Copyright 2010-2018 Canonical Ltd. This software is licensed under the |
708 | +# Copyright 2010-2019 Canonical Ltd. This software is licensed under the |
709 | # GNU Affero General Public License version 3 (see the file LICENSE). |
710 | |
711 | """Tests for sync package jobs.""" |
712 | @@ -220,7 +220,7 @@ class PlainPackageCopyJobTests(TestCaseWithFactory, LocalTestHelper): |
713 | package_version="1.0-1", include_binaries=False, |
714 | copy_policy=PackageCopyPolicy.MASS_SYNC, |
715 | requester=requester, sponsored=sponsored, |
716 | - phased_update_percentage=20) |
717 | + phased_update_percentage=20, move=True) |
718 | self.assertProvides(job, IPackageCopyJob) |
719 | self.assertEqual(archive1.id, job.source_archive_id) |
720 | self.assertEqual(archive1, job.source_archive) |
721 | @@ -230,11 +230,12 @@ class PlainPackageCopyJobTests(TestCaseWithFactory, LocalTestHelper): |
722 | self.assertEqual(PackagePublishingPocket.RELEASE, job.target_pocket) |
723 | self.assertEqual("foo", job.package_name) |
724 | self.assertEqual("1.0-1", job.package_version) |
725 | - self.assertEqual(False, job.include_binaries) |
726 | + self.assertFalse(job.include_binaries) |
727 | self.assertEqual(PackageCopyPolicy.MASS_SYNC, job.copy_policy) |
728 | self.assertEqual(requester, job.requester) |
729 | self.assertEqual(sponsored, job.sponsored) |
730 | self.assertEqual(20, job.phased_update_percentage) |
731 | + self.assertTrue(job.move) |
732 | |
733 | def test_createMultiple_creates_one_job_per_copy(self): |
734 | mother = self.factory.makeDistroSeriesParent() |
735 | @@ -1726,6 +1727,30 @@ class PlainPackageCopyJobTests(TestCaseWithFactory, LocalTestHelper): |
736 | 1, archive.getPublishedOnDiskBinaries( |
737 | status=PackagePublishingStatus.PENDING).count()) |
738 | |
739 | + def test_move(self): |
740 | + # A job with move=True deletes the old publication after copying it. |
741 | + source_archive = self.factory.makeArchive( |
742 | + self.distroseries.distribution) |
743 | + target_archive = self.factory.makeArchive( |
744 | + self.distroseries.distribution) |
745 | + spph = self.publisher.getPubSource( |
746 | + distroseries=self.distroseries, sourcename="moveme", |
747 | + archive=source_archive) |
748 | + with person_logged_in(target_archive.owner): |
749 | + target_archive.newComponentUploader(source_archive.owner, "main") |
750 | + job = self.createCopyJobForSPPH( |
751 | + spph, source_archive, target_archive, |
752 | + requester=source_archive.owner, move=True) |
753 | + self.runJob(job) |
754 | + self.assertEqual(JobStatus.COMPLETED, job.status) |
755 | + new_spph = target_archive.getPublishedSources(name="moveme").one() |
756 | + self.assertEqual(PackagePublishingStatus.PENDING, new_spph.status) |
757 | + self.assertEqual(PackagePublishingStatus.DELETED, spph.status) |
758 | + self.assertEqual( |
759 | + "Moved to %s in %s" % ( |
760 | + self.distroseries.name, target_archive.reference), |
761 | + spph.removal_comment) |
762 | + |
763 | |
764 | class TestViaCelery(TestCaseWithFactory): |
765 | """PackageCopyJob runs under Celery.""" |
766 | diff --git a/lib/lp/soyuz/vocabularies.py b/lib/lp/soyuz/vocabularies.py |
767 | index b4f40b5..bcaf16e 100644 |
768 | --- a/lib/lp/soyuz/vocabularies.py |
769 | +++ b/lib/lp/soyuz/vocabularies.py |
770 | @@ -1,4 +1,4 @@ |
771 | -# Copyright 2009-2018 Canonical Ltd. This software is licensed under the GNU |
772 | +# Copyright 2009-2019 Canonical Ltd. This software is licensed under the GNU |
773 | # Affero General Public License version 3 (see the file LICENSE). |
774 | |
775 | """Soyuz vocabularies.""" |
776 | @@ -8,6 +8,7 @@ __metaclass__ = type |
777 | __all__ = [ |
778 | 'ComponentVocabulary', |
779 | 'FilteredDistroArchSeriesVocabulary', |
780 | + 'make_archive_vocabulary', |
781 | 'PackageReleaseVocabulary', |
782 | 'PPAVocabulary', |
783 | ] |
784 | @@ -18,7 +19,10 @@ from storm.locals import ( |
785 | ) |
786 | from zope.component import getUtility |
787 | from zope.interface import implementer |
788 | -from zope.schema.vocabulary import SimpleTerm |
789 | +from zope.schema.vocabulary import ( |
790 | + SimpleTerm, |
791 | + SimpleVocabulary, |
792 | + ) |
793 | from zope.security.interfaces import Unauthorized |
794 | |
795 | from lp.registry.model.distroseries import DistroSeries |
796 | @@ -150,3 +154,12 @@ class PPAVocabulary(SQLObjectVocabularyBase): |
797 | search_clause) |
798 | return self._table.select( |
799 | clause, orderBy=self._orderBy, clauseTables=self._clauseTables) |
800 | + |
801 | + |
802 | +def make_archive_vocabulary(archives): |
803 | + terms = [] |
804 | + for archive in archives: |
805 | + label = '%s [%s]' % (archive.displayname, archive.reference) |
806 | + terms.append(SimpleTerm(archive, archive.reference, label)) |
807 | + terms.sort(key=lambda x: x.value.reference) |
808 | + return SimpleVocabulary(terms) |
> "[this] is ~800 lines of code motion [and test] and passing parameters down through multiple layers and 6 lines of actually doing the removal after the copy if instructed to do so
Yes, yes it is!
And it looks good to me