launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #09572
[Merge] lp:~cjwatson/launchpad/queue-api-overrides into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/queue-api-overrides into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/queue-api-overrides/+merge/113437
== Summary ==
Replace the queue tool in Launchpad with an API client (part five).
== Proposed fix ==
This branch exports the override methods on PackageUpload. With this in addition to what's already landed, the client should be feature-complete, and the remaining work should consist of cleanups.
== Pre-implementation notes ==
I've gone round a few times with various people, particularly William Grant, on the exact way to export all of this stuff, because I gather that we want to avoid exposing the current data model in order that it can be rearranged in the future. This has led to the following design choices:
* Everything is on devel. The only clients for this should be tools such as those in lp:ubuntu-archive-tools, which can be kept up to date if there's a need to change these interfaces.
* There are source packages with lots of binaries that sometimes need to be overridden individually (e.g. linux) and API requests aren't especially fast. In a previous branch (currently QAed and awaiting deployment) I amended Archive.overrideBinaries to take a similar list of dicts as a "changes" parameter, allowing many override changes to be made in a single request; this exports that and improves test coverage.
I extracted this branch from https://code.launchpad.net/~cjwatson/launchpad/queue-api/+merge/108967 at Benji's request.
== Implementation details ==
The only thing I think is notable here is that I copied the new-component-requires-new-archive check from lib/lp/soyuz/scripts/queue.py into PackageUpload.overrideBinaries. This should be acceptable temporary duplication since: (a) it really belongs in the model anyway; (b) lp.soyuz.scripts.queue calls BPR.override directly rather than going through PackageUpload.overrideBinaries, so the check doesn't happen twice at run-time; (c) I will be removing lp.soyuz.scripts.queue soon so there's no point bothering to refactor it.
== LOC Rationale ==
+218. As with https://code.launchpad.net/~cjwatson/launchpad/queue-api/+merge/108967, this arc will be LoC-negative.
== Tests ==
bin/test -vvct test_packageupload
== Demo and Q/A ==
http://paste.ubuntu.com/1072964/ is the current version of my client. With this branch, it should be possible to override sources and (individual) binaries in queue items.
--
https://code.launchpad.net/~cjwatson/launchpad/queue-api-overrides/+merge/113437
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/queue-api-overrides into lp:launchpad.
=== modified file 'lib/lp/soyuz/doc/distroseriesqueue.txt'
--- lib/lp/soyuz/doc/distroseriesqueue.txt 2012-07-03 09:29:35 +0000
+++ lib/lp/soyuz/doc/distroseriesqueue.txt 2012-07-04 17:21:19 +0000
@@ -648,7 +648,7 @@
In addition to these parameters, you must also supply
"allowed_components", which is a sequence of IComponent. Any overrides
must have the existing and new component in this sequence otherwise
-QueueInconsistentStateError is raised.
+QueueAdminUnauthorizedError is raised.
The alsa-utils source is already in the queue with component "main"
and section "base".
@@ -673,7 +673,7 @@
... allowed_components=(universe,))
Traceback (most recent call last):
...
- QueueInconsistentStateError: No rights to override to restricted
+ QueueAdminUnauthorizedError: No rights to override to restricted
Allowing "restricted" still won't work because the original component
is "main":
@@ -683,7 +683,7 @@
... allowed_components=(restricted,))
Traceback (most recent call last):
...
- QueueInconsistentStateError: No rights to override from main
+ QueueAdminUnauthorizedError: No rights to override from main
Specifying both main and restricted allows the override to restricted/web.
overrideSource() returns True if it completed the task.
@@ -719,13 +719,13 @@
... binary_changes, allowed_components=(universe,))
Traceback (most recent call last):
...
- QueueInconsistentStateError: No rights to override to restricted
+ QueueAdminUnauthorizedError: No rights to override to restricted
>>> print item.overrideBinaries(
... binary_changes, allowed_components=(restricted,))
Traceback (most recent call last):
...
- QueueInconsistentStateError: No rights to override from main
+ QueueAdminUnauthorizedError: No rights to override from main
>>> print item.overrideBinaries(
... binary_changes, allowed_components=(main,restricted))
=== modified file 'lib/lp/soyuz/interfaces/queue.py'
--- lib/lp/soyuz/interfaces/queue.py 2012-07-04 13:02:32 +0000
+++ lib/lp/soyuz/interfaces/queue.py 2012-07-04 17:21:19 +0000
@@ -16,6 +16,7 @@
'IPackageUploadCustom',
'IPackageUploadSet',
'NonBuildableSourceUploadError',
+ 'QueueAdminUnauthorizedError',
'QueueBuildAcceptError',
'QueueInconsistentStateError',
'QueueSourceAcceptError',
@@ -26,12 +27,15 @@
from lazr.enum import DBEnumeratedType
from lazr.restful.declarations import (
+ call_with,
error_status,
export_as_webservice_entry,
export_read_operation,
export_write_operation,
exported,
operation_for_version,
+ operation_parameters,
+ REQUEST_USER,
)
from lazr.restful.fields import Reference
from zope.interface import (
@@ -42,10 +46,12 @@
Bool,
Choice,
Datetime,
+ Dict,
Int,
List,
TextLine,
)
+from zope.security.interfaces import Unauthorized
from lp import _
from lp.soyuz.enums import PackageUploadStatus
@@ -69,6 +75,10 @@
"""
+class QueueAdminUnauthorizedError(Unauthorized):
+ """User not permitted to perform a queue administration operation."""
+
+
class NonBuildableSourceUploadError(QueueInconsistentStateError):
"""Source upload will not result in any build record.
@@ -394,7 +404,14 @@
:param logger: Specify a logger object if required. Mainly for tests.
"""
- def overrideSource(new_component, new_section, allowed_components):
+ @operation_parameters(
+ new_component=TextLine(title=u"The new component name."),
+ new_section=TextLine(title=u"The new section name."))
+ @call_with(allowed_components=None, user=REQUEST_USER)
+ @export_write_operation()
+ @operation_for_version('devel')
+ def overrideSource(new_component=None, new_section=None,
+ allowed_components=None, user=None):
"""Override the source package contained in this queue item.
:param new_component: An IComponent to replace the existing one
@@ -403,6 +420,8 @@
in the upload's source.
:param allowed_components: A sequence of components that the
callsite is allowed to override from and to.
+ :param user: The user requesting the override change, used if
+ allowed_components is None.
:raises QueueInconsistentStateError: if either the existing
or the new_component are not in the allowed_components
@@ -414,7 +433,21 @@
:return: True if the source was overridden.
"""
- def overrideBinaries(changes, allowed_components):
+ @operation_parameters(
+ changes=List(
+ title=u"A sequence of changes to apply.",
+ description=(
+ u"Each item may have a 'name' item which specifies the binary "
+ "package name to override; otherwise, the change applies to "
+ "all binaries in the upload. It may also have 'component', "
+ "'section', and 'priority' items which replace the "
+ "corresponding existing one in the upload's overridden "
+ "binaries."),
+ value_type=Dict(key_type=TextLine())))
+ @call_with(allowed_components=None, user=REQUEST_USER)
+ @export_write_operation()
+ @operation_for_version('devel')
+ def overrideBinaries(changes, allowed_components=None, user=None):
"""Override binary packages in a binary queue item.
:param changes: A sequence of mappings of changes to apply. Each
@@ -426,6 +459,8 @@
left unchanged.
:param allowed_components: A sequence of components that the
callsite is allowed to override from and to.
+ :param user: The user requesting the override change, used if
+ allowed_components is None.
:raises QueueInconsistentStateError: if either the existing
or the new_component are not in the allowed_components
=== modified file 'lib/lp/soyuz/model/queue.py'
--- lib/lp/soyuz/model/queue.py 2012-07-04 13:02:32 +0000
+++ lib/lp/soyuz/model/queue.py 2012-07-04 17:21:19 +0000
@@ -85,6 +85,7 @@
PriorityNotFound,
SectionNotFound,
)
+from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet
from lp.soyuz.interfaces.component import IComponentSet
from lp.soyuz.interfaces.packagecopyjob import IPackageCopyJobSource
from lp.soyuz.interfaces.publishing import (
@@ -100,6 +101,7 @@
IPackageUploadSet,
IPackageUploadSource,
NonBuildableSourceUploadError,
+ QueueAdminUnauthorizedError,
QueueBuildAcceptError,
QueueInconsistentStateError,
QueueSourceAcceptError,
@@ -1028,7 +1030,7 @@
allowed_component_names = [
component.name for component in allowed_components]
if copy_job.component_name not in allowed_component_names:
- raise QueueInconsistentStateError(
+ raise QueueAdminUnauthorizedError(
"No rights to override from %s" % copy_job.component_name)
copy_job.addSourceOverride(SourceOverride(
copy_job.package_name, new_component, new_section))
@@ -1045,7 +1047,7 @@
if old_component not in allowed_components:
# The old component is not in the list of allowed components
# to override.
- raise QueueInconsistentStateError(
+ raise QueueAdminUnauthorizedError(
"No rights to override from %s" % old_component.name)
source.sourcepackagerelease.override(
component=new_component, section=new_section)
@@ -1058,7 +1060,8 @@
return made_changes
- def overrideSource(self, new_component, new_section, allowed_components):
+ def overrideSource(self, new_component=None, new_section=None,
+ allowed_components=None, user=None):
"""See `IPackageUpload`."""
if new_component is None and new_section is None:
# Nothing needs overriding, bail out.
@@ -1067,8 +1070,19 @@
new_component = self._nameToComponent(new_component)
new_section = self._nameToSection(new_section)
+ if allowed_components is None and user is not None:
+ # Get a list of components for which the user has rights to
+ # override to or from.
+ permission_set = getUtility(IArchivePermissionSet)
+ permissions = permission_set.componentsForQueueAdmin(
+ self.distroseries.main_archive, user)
+ allowed_components = set(
+ permission.component for permission in permissions)
+ assert allowed_components is not None, (
+ "Must provide allowed_components for non-webservice calls.")
+
if new_component not in list(allowed_components) + [None]:
- raise QueueInconsistentStateError(
+ raise QueueAdminUnauthorizedError(
"No rights to override to %s" % new_component.name)
return (
@@ -1103,7 +1117,7 @@
return changes_by_name, changes_for_all
- def overrideBinaries(self, changes, allowed_components):
+ def overrideBinaries(self, changes, allowed_components=None, user=None):
"""See `IPackageUpload`."""
if not self.contains_build:
return False
@@ -1112,6 +1126,17 @@
# Nothing needs overriding, bail out.
return False
+ if allowed_components is None and user is not None:
+ # Get a list of components for which the user has rights to
+ # override to or from.
+ permission_set = getUtility(IArchivePermissionSet)
+ permissions = permission_set.componentsForQueueAdmin(
+ self.distroseries.main_archive, user)
+ allowed_components = set(
+ permission.component for permission in permissions)
+ assert allowed_components is not None, (
+ "Must provide allowed_components for non-webservice calls.")
+
changes_by_name, changes_for_all = self._filterBinaryChanges(changes)
new_components = set()
@@ -1125,12 +1150,23 @@
component.name
for component in new_components.difference(allowed_components))
if disallowed_components:
- raise QueueInconsistentStateError(
+ raise QueueAdminUnauthorizedError(
"No rights to override to %s" %
", ".join(disallowed_components))
made_changes = False
for build in self.builds:
+ # See if the new component requires a new archive on the build.
+ for component in new_components:
+ distroarchseries = build.build.distro_arch_series
+ distribution = distroarchseries.distroseries.distribution
+ new_archive = distribution.getArchiveByComponent(
+ component.name)
+ if new_archive != build.build.archive:
+ raise QueueInconsistentStateError(
+ "Overriding component to '%s' failed because it "
+ "would require a new archive." % component.name)
+
for binarypackage in build.build.binarypackages:
change = changes_by_name.get(
binarypackage.name, changes_for_all)
@@ -1138,7 +1174,7 @@
if binarypackage.component not in allowed_components:
# The old component is not in the list of allowed
# components to override.
- raise QueueInconsistentStateError(
+ raise QueueAdminUnauthorizedError(
"No rights to override from %s" %
binarypackage.component.name)
binarypackage.override(**change)
=== modified file 'lib/lp/soyuz/tests/test_packageupload.py'
--- lib/lp/soyuz/tests/test_packageupload.py 2012-07-04 13:02:32 +0000
+++ lib/lp/soyuz/tests/test_packageupload.py 2012-07-04 17:21:19 +0000
@@ -42,6 +42,7 @@
from lp.soyuz.interfaces.queue import (
IPackageUpload,
IPackageUploadSet,
+ QueueAdminUnauthorizedError,
QueueInconsistentStateError,
)
from lp.soyuz.interfaces.section import ISectionSet
@@ -444,8 +445,7 @@
only_allowed_component = self.factory.makeComponent()
section = self.factory.makeSection()
self.assertRaises(
- QueueInconsistentStateError,
- pu.overrideSource,
+ QueueAdminUnauthorizedError, pu.overrideSource,
only_allowed_component, section, [only_allowed_component])
def test_overrideSource_checks_permission_for_new_component(self):
@@ -454,8 +454,7 @@
disallowed_component = self.factory.makeComponent()
section = self.factory.makeSection()
self.assertRaises(
- QueueInconsistentStateError,
- pu.overrideSource,
+ QueueAdminUnauthorizedError, pu.overrideSource,
disallowed_component, section, [current_component])
def test_overrideSource_ignores_None_component_change(self):
@@ -971,6 +970,9 @@
def test_edit_permissions(self):
self.assertRequiresEdit("acceptFromQueue")
self.assertRequiresEdit("rejectFromQueue")
+ self.assertRequiresEdit("overrideSource", new_component="main")
+ self.assertRequiresEdit(
+ "overrideBinaries", changes=[{"component": "main"}])
def test_acceptFromQueue_archive_admin(self):
# acceptFromQueue as an archive admin accepts the upload.
@@ -1034,6 +1036,45 @@
for file in upload.sourcepackagerelease.files]
self.assertContentEqual(source_file_urls, ws_source_file_urls)
+ def test_overrideSource_limited_component_permissions(self):
+ # Overriding between two components requires queue admin of both.
+ person = self.makeQueueAdmin([self.universe])
+ upload, ws_upload = self.makeSourcePackageUpload(
+ person, component=self.universe)
+
+ self.assertEqual("New", ws_upload.status)
+ self.assertEqual("universe", ws_upload.component_name)
+ self.assertRaises(Unauthorized, ws_upload.overrideSource,
+ new_component="main")
+
+ with admin_logged_in():
+ upload.overrideSource(
+ new_component=self.main,
+ allowed_components=[self.main, self.universe])
+ transaction.commit()
+ self.assertEqual("main", upload.component_name)
+ self.assertRaises(Unauthorized, ws_upload.overrideSource,
+ new_component="universe")
+
+ def test_overrideSource_changes_properties(self):
+ # Running overrideSource changes the corresponding properties.
+ person = self.makeQueueAdmin([self.main, self.universe])
+ upload, ws_upload = self.makeSourcePackageUpload(
+ person, component=self.universe)
+ with person_logged_in(person):
+ new_section = self.factory.makeSection()
+ transaction.commit()
+
+ self.assertEqual("New", ws_upload.status)
+ self.assertEqual("universe", ws_upload.component_name)
+ self.assertNotEqual(new_section.name, ws_upload.section_name)
+ ws_upload.overrideSource(
+ new_component="main", new_section=new_section.name)
+ self.assertEqual("main", ws_upload.component_name)
+ self.assertEqual(new_section.name, ws_upload.section_name)
+ ws_upload.overrideSource(new_component="universe")
+ self.assertEqual("universe", ws_upload.component_name)
+
def test_binary_info(self):
# API clients can inspect properties of binary uploads.
person = self.makeQueueAdmin([self.universe])
@@ -1077,6 +1118,112 @@
for file in bpr.files]
self.assertContentEqual(binary_file_urls, ws_binary_file_urls)
+ def test_overrideBinaries_limited_component_permissions(self):
+ # Overriding between two components requires queue admin of both.
+ person = self.makeQueueAdmin([self.universe])
+ upload, ws_upload = self.makeBinaryPackageUpload(
+ person, binarypackagename="hello", component=self.universe)
+
+ self.assertEqual("New", ws_upload.status)
+ self.assertEqual(
+ set(["universe"]),
+ set(binary["component"]
+ for binary in ws_upload.getBinaryProperties()))
+ self.assertRaises(
+ Unauthorized, ws_upload.overrideBinaries,
+ changes=[{"component": "main"}])
+
+ with admin_logged_in():
+ upload.overrideBinaries(
+ [{"component": self.main}],
+ allowed_components=[self.main, self.universe])
+ transaction.commit()
+
+ self.assertEqual(
+ set(["main"]),
+ set(binary["component"]
+ for binary in ws_upload.getBinaryProperties()))
+ self.assertRaises(
+ Unauthorized, ws_upload.overrideBinaries,
+ changes=[{"component": "universe"}])
+
+ def test_overrideBinaries_disallows_new_archive(self):
+ # overrideBinaries refuses to override the component to something
+ # that requires a different archive.
+ partner = self.factory.makeComponent("partner")
+ self.factory.makeComponentSelection(
+ distroseries=self.distroseries, component=partner)
+ person = self.makeQueueAdmin([self.universe, partner])
+ upload, ws_upload = self.makeBinaryPackageUpload(
+ person, component=self.universe)
+
+ self.assertEqual(
+ "universe", ws_upload.getBinaryProperties()[0]["component"])
+ self.assertRaises(
+ BadRequest, ws_upload.overrideBinaries,
+ changes=[{"component": "partner"}])
+
+ def test_overrideBinaries_without_name_changes_all_properties(self):
+ # Running overrideBinaries with a change entry containing no "name"
+ # field changes the corresponding properties of all binaries.
+ person = self.makeQueueAdmin([self.main, self.universe])
+ upload, ws_upload = self.makeBinaryPackageUpload(
+ person, component=self.universe)
+ with person_logged_in(person):
+ new_section = self.factory.makeSection()
+ transaction.commit()
+
+ self.assertEqual("New", ws_upload.status)
+ for binary in ws_upload.getBinaryProperties():
+ self.assertEqual("universe", binary["component"])
+ self.assertNotEqual(new_section.name, binary["section"])
+ self.assertEqual("OPTIONAL", binary["priority"])
+ changes = [{
+ "component": "main",
+ "section": new_section.name,
+ "priority": "extra",
+ }]
+ ws_upload.overrideBinaries(changes=changes)
+ for binary in ws_upload.getBinaryProperties():
+ self.assertEqual("main", binary["component"])
+ self.assertEqual(new_section.name, binary["section"])
+ self.assertEqual("EXTRA", binary["priority"])
+
+ def test_overrideBinaries_with_name_changes_selected_properties(self):
+ # Running overrideBinaries with change entries containing "name"
+ # fields changes the corresponding properties of only the selected
+ # binaries.
+ person = self.makeQueueAdmin([self.main, self.universe])
+ upload, ws_upload = self.makeBinaryPackageUpload(
+ person, component=self.universe)
+ with person_logged_in(person):
+ new_section = self.factory.makeSection()
+ transaction.commit()
+
+ self.assertEqual("New", ws_upload.status)
+ ws_binaries = ws_upload.getBinaryProperties()
+ for binary in ws_binaries:
+ self.assertEqual("universe", binary["component"])
+ self.assertNotEqual(new_section.name, binary["section"])
+ self.assertEqual("OPTIONAL", binary["priority"])
+ change_one = {
+ "name": ws_binaries[0]["name"],
+ "component": "main",
+ "priority": "standard",
+ }
+ change_two = {
+ "name": ws_binaries[1]["name"],
+ "section": new_section.name,
+ }
+ ws_upload.overrideBinaries(changes=[change_one, change_two])
+ ws_binaries = ws_upload.getBinaryProperties()
+ self.assertEqual("main", ws_binaries[0]["component"])
+ self.assertNotEqual(new_section.name, ws_binaries[0]["section"])
+ self.assertEqual("STANDARD", ws_binaries[0]["priority"])
+ self.assertEqual("universe", ws_binaries[1]["component"])
+ self.assertEqual(new_section.name, ws_binaries[1]["section"])
+ self.assertEqual("OPTIONAL", ws_binaries[1]["priority"])
+
def test_custom_info(self):
# API clients can inspect properties of custom uploads.
person = self.makeQueueAdmin([self.universe])
Follow ups
