Merge lp:~stevenk/launchpad/db-add-derivedistroseries-api into lp:launchpad/db-devel
- db-add-derivedistroseries-api
- Merge into db-devel
Proposed by
Steve Kowalik
| Status: | Rejected | ||||
|---|---|---|---|---|---|
| Rejected by: | Steve Kowalik | ||||
| Proposed branch: | lp:~stevenk/launchpad/db-add-derivedistroseries-api | ||||
| Merge into: | lp:launchpad/db-devel | ||||
| Diff against target: |
805 lines (+449/-26) 12 files modified
lib/lp/registry/interfaces/distroseries.py (+80/-4) lib/lp/registry/model/distroseries.py (+50/-0) lib/lp/registry/stories/webservice/xx-derivedistroseries.txt (+68/-0) lib/lp/registry/tests/test_derivedistroseries.py (+78/-0) lib/lp/soyuz/configure.zcml (+5/-2) lib/lp/soyuz/interfaces/distributionjob.py (+1/-1) lib/lp/soyuz/interfaces/packagecloner.py (+5/-2) lib/lp/soyuz/model/initialisedistroseriesjob.py (+21/-3) lib/lp/soyuz/model/packagecloner.py (+46/-8) lib/lp/soyuz/scripts/initialise_distroseries.py (+17/-3) lib/lp/soyuz/scripts/tests/test_initialise_distroseries.py (+62/-3) lib/lp/soyuz/tests/test_initialisedistroseriesjob.py (+16/-0) |
||||
| To merge this branch: | bzr merge lp:~stevenk/launchpad/db-add-derivedistroseries-api | ||||
| Related bugs: |
|
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Gavin Panella (community) | Approve | ||
|
Review via email:
|
|||
Commit message
Add a new method to IDistroSeries, deriveDistroSer
Description of the change
This branch adds a new method to IDistroSeries: deriveDistroSer
This method is intended to be called via API scripts for both the distro use-case as well as the Linaro use-case to initialise a new distroseries (which may or may not exist), from an existing distroseries. It will create the new distroseries if it doesn't exist, under the specified distribution, unless it isn't specified, in which case it will use the current distroseries's distribution (.parent in the interface).
After it has verified everything, and created the distroseries if need be, it checks the details submitted via the InitialiseDistr
I have written two seperate tests, one that calls the method directly and tests it, and a lighter doctest that checks via the API.
I have discussed this change with both Julian and Michael, and they agreed that it sounded good.
To test: bin/test -vvt test_derivedist
This branch also drive-bys two mis-spellings of distribution, since I noticed them.
To post a comment you must log in.
Revision history for this message
| Gavin Panella (allenap) wrote : | # |
As discussed, [3] is okay to leave as long as bigjools is happy with the approach, though I might talk to someone better versed in Zope to ensure that I haven't missed something.
Revision history for this message
| Gavin Panella (allenap) wrote : | # |
Cool, the changes look good.
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
| 1 | === modified file 'lib/lp/registry/interfaces/distroseries.py' |
| 2 | --- lib/lp/registry/interfaces/distroseries.py 2010-09-28 18:11:41 +0000 |
| 3 | +++ lib/lp/registry/interfaces/distroseries.py 2010-10-12 08:34:50 +0000 |
| 4 | @@ -8,6 +8,7 @@ |
| 5 | __metaclass__ = type |
| 6 | |
| 7 | __all__ = [ |
| 8 | + 'DerivationError', |
| 9 | 'IDistroSeries', |
| 10 | 'IDistroSeriesEditRestricted', |
| 11 | 'IDistroSeriesPublic', |
| 12 | @@ -16,20 +17,25 @@ |
| 13 | |
| 14 | from lazr.enum import DBEnumeratedType |
| 15 | from lazr.restful.declarations import ( |
| 16 | + call_with, |
| 17 | export_as_webservice_entry, |
| 18 | export_factory_operation, |
| 19 | export_read_operation, |
| 20 | + export_write_operation, |
| 21 | exported, |
| 22 | LAZR_WEBSERVICE_EXPORTED, |
| 23 | operation_parameters, |
| 24 | operation_returns_collection_of, |
| 25 | operation_returns_entry, |
| 26 | rename_parameters_as, |
| 27 | + REQUEST_USER, |
| 28 | + webservice_error, |
| 29 | ) |
| 30 | from lazr.restful.fields import ( |
| 31 | Reference, |
| 32 | ReferenceChoice, |
| 33 | ) |
| 34 | +from lazr.restful.interface import copy_field |
| 35 | from zope.component import getUtility |
| 36 | from zope.interface import ( |
| 37 | Attribute, |
| 38 | @@ -39,6 +45,7 @@ |
| 39 | Bool, |
| 40 | Choice, |
| 41 | Datetime, |
| 42 | + List, |
| 43 | Object, |
| 44 | TextLine, |
| 45 | ) |
| 46 | @@ -673,8 +680,8 @@ |
| 47 | If sourcename is passed, only packages that are built from |
| 48 | source packages by that name will be returned. |
| 49 | If archive is passed, restricted the results to the given archive, |
| 50 | - if it is suppressed the results will be restricted to the distribtion |
| 51 | - 'main_archive'. |
| 52 | + if it is suppressed the results will be restricted to the |
| 53 | + distribution 'main_archive'. |
| 54 | """ |
| 55 | |
| 56 | def getSourcePackagePublishing(status, pocket, component=None, |
| 57 | @@ -683,8 +690,8 @@ |
| 58 | |
| 59 | According status and pocket. |
| 60 | If archive is passed, restricted the results to the given archive, |
| 61 | - if it is suppressed the results will be restricted to the distribtion |
| 62 | - 'main_archive'. |
| 63 | + if it is suppressed the results will be restricted to the |
| 64 | + distribution 'main_archive'. |
| 65 | """ |
| 66 | |
| 67 | def getBinaryPackageCaches(archive=None): |
| 68 | @@ -789,6 +796,69 @@ |
| 69 | :param format: The SourcePackageFormat to check. |
| 70 | """ |
| 71 | |
| 72 | + @operation_parameters( |
| 73 | + name=copy_field(name, required=True), |
| 74 | + displayname=copy_field(displayname, required=False), |
| 75 | + title=copy_field(title, required=False), |
| 76 | + summary=TextLine( |
| 77 | + title=_("The summary of the distroseries to derive."), |
| 78 | + required=False), |
| 79 | + description=copy_field(description, required=False), |
| 80 | + version=copy_field(version, required=False), |
| 81 | + distribution=copy_field(distribution, required=False), |
| 82 | + status=copy_field(status, required=False), |
| 83 | + architectures=List( |
| 84 | + title=_("The list of architectures to copy to the derived " |
| 85 | + "distroseries."), |
| 86 | + required=False), |
| 87 | + packagesets=List( |
| 88 | + title=_("The list of packagesets to copy to the derived " |
| 89 | + "distroseries"), |
| 90 | + required=False), |
| 91 | + rebuild=Bool( |
| 92 | + title=_("If binaries will be copied to the derived " |
| 93 | + "distroseries."), |
| 94 | + required=True), |
| 95 | + ) |
| 96 | + @call_with(user=REQUEST_USER) |
| 97 | + @export_write_operation() |
| 98 | + def deriveDistroSeries( |
| 99 | + user, name, displayname, title, summary, description, version, |
| 100 | + distribution, status, architectures, packagesets, rebuild): |
| 101 | + """Derive a distroseries from this one. |
| 102 | + |
| 103 | + This method performs checks, can create the new distroseries if |
| 104 | + necessary, and then creates a job to populate the new |
| 105 | + distroseries. |
| 106 | + |
| 107 | + :param name: The name of the new distroseries we will create if it |
| 108 | + doesn't exist, or the name of the distroseries we will look |
| 109 | + up, and then initialise. |
| 110 | + :param displayname: The Display Name for the new distroseries. |
| 111 | + If the distroseries already exists this parameter is ignored. |
| 112 | + :param title: The Title for the new distroseries. If the |
| 113 | + distroseries already exists this parameter is ignored. |
| 114 | + :param summary: The Summary for the new distroseries. If the |
| 115 | + distroseries already exists this parameter is ignored. |
| 116 | + :param description: The Description for the new distroseries. If the |
| 117 | + distroseries already exists this parameter is ignored. |
| 118 | + :param version: The version for the new distroseries. If the |
| 119 | + distroseries already exists this parameter is ignored. |
| 120 | + :param distribution: The distribution the derived series will |
| 121 | + belong to. If it isn't specified this distroseries' |
| 122 | + distribution is used. |
| 123 | + :param status: The status the new distroseries will be created |
| 124 | + in. If the distroseries isn't specified, this parameter will |
| 125 | + be ignored. Defaults to FROZEN. |
| 126 | + :param architectures: The architectures to copy to the derived |
| 127 | + series. If not specified, all of the architectures are copied. |
| 128 | + :param packagesets: The packagesets to copy to the derived series. |
| 129 | + If not specified, all of the packagesets are copied. |
| 130 | + :param rebuild: Whether binaries will be copied to the derived |
| 131 | + series. If it's true, they will not be, and if it's false, they |
| 132 | + will be. |
| 133 | + """ |
| 134 | + |
| 135 | |
| 136 | class IDistroSeries(IDistroSeriesEditRestricted, IDistroSeriesPublic, |
| 137 | IStructuralSubscriptionTarget): |
| 138 | @@ -856,5 +926,11 @@ |
| 139 | """ |
| 140 | |
| 141 | |
| 142 | +class DerivationError(Exception): |
| 143 | + """Raised when there is a problem deriving a distroseries.""" |
| 144 | + webservice_error(400) # Bad Request |
| 145 | + _message_prefix = "Error deriving distro series" |
| 146 | + |
| 147 | + |
| 148 | # Monkey patch for circular import avoidance done in |
| 149 | # _schema_circular_imports.py |
| 150 | |
| 151 | === modified file 'lib/lp/registry/model/distroseries.py' |
| 152 | --- lib/lp/registry/model/distroseries.py 2010-10-03 15:30:06 +0000 |
| 153 | +++ lib/lp/registry/model/distroseries.py 2010-10-12 08:34:50 +0000 |
| 154 | @@ -35,6 +35,7 @@ |
| 155 | ) |
| 156 | from zope.component import getUtility |
| 157 | from zope.interface import implements |
| 158 | +from zope.security.interfaces import Unauthorized |
| 159 | |
| 160 | from canonical.database.constants import ( |
| 161 | DEFAULT, |
| 162 | @@ -88,6 +89,7 @@ |
| 163 | ) |
| 164 | from lp.bugs.model.bugtask import BugTask |
| 165 | from lp.registry.interfaces.distroseries import ( |
| 166 | + DerivationError, |
| 167 | IDistroSeries, |
| 168 | IDistroSeriesSet, |
| 169 | ) |
| 170 | @@ -128,6 +130,9 @@ |
| 171 | from lp.soyuz.interfaces.binarypackagebuild import IBinaryPackageBuildSet |
| 172 | from lp.soyuz.interfaces.binarypackagename import IBinaryPackageName |
| 173 | from lp.soyuz.interfaces.buildrecords import IHasBuildRecords |
| 174 | +from lp.soyuz.interfaces.distributionjob import ( |
| 175 | + IInitialiseDistroSeriesJobSource, |
| 176 | + ) |
| 177 | from lp.soyuz.interfaces.publishing import ( |
| 178 | active_publishing_status, |
| 179 | ICanPublishPackages, |
| 180 | @@ -162,6 +167,10 @@ |
| 181 | ) |
| 182 | from lp.soyuz.model.section import Section |
| 183 | from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease |
| 184 | +from lp.soyuz.scripts.initialise_distroseries import ( |
| 185 | + InitialisationError, |
| 186 | + InitialiseDistroSeries, |
| 187 | + ) |
| 188 | from lp.translations.interfaces.languagepack import LanguagePackType |
| 189 | from lp.translations.model.distroseries_translations_copy import ( |
| 190 | copy_active_translations, |
| 191 | @@ -1847,6 +1856,47 @@ |
| 192 | ISourcePackageFormatSelectionSet).getBySeriesAndFormat( |
| 193 | self, format) is not None |
| 194 | |
| 195 | + def deriveDistroSeries( |
| 196 | + self, user, name, distribution=None, displayname=None, |
| 197 | + title=None, summary=None, description=None, version=None, |
| 198 | + status=SeriesStatus.FROZEN, architectures=(), packagesets=(), |
| 199 | + rebuild=False): |
| 200 | + """See `IDistroSeries`.""" |
| 201 | + # XXX StevenK bug=643369 This should be in the security adapter |
| 202 | + # This should be allowed if the user is a driver for self.parent |
| 203 | + # or the child.parent's drivers. |
| 204 | + if not (user.inTeam('soyuz-team') or user.inTeam('admins')): |
| 205 | + raise Unauthorized |
| 206 | + child = IStore(self).find(DistroSeries, name=name).one() |
| 207 | + if child is None: |
| 208 | + if distribution is None: |
| 209 | + distribution = self.distribution |
| 210 | + for param in ( |
| 211 | + displayname, title, summary, description, version): |
| 212 | + if param is None or len(param) == 0: |
| 213 | + raise DerivationError( |
| 214 | + "Display Name, Title, Summary, Description and" |
| 215 | + " Version all need to be set when creating a" |
| 216 | + " distroseries") |
| 217 | + child = distribution.newSeries( |
| 218 | + name=name, displayname=displayname, title=title, |
| 219 | + summary=summary, description=description, |
| 220 | + version=version, parent_series=self, owner=user) |
| 221 | + child.status = status |
| 222 | + IStore(self).add(child) |
| 223 | + else: |
| 224 | + if child.parent_series is not self: |
| 225 | + raise DerivationError( |
| 226 | + "DistroSeries %s parent series isn't %s" % ( |
| 227 | + child.name, self.name)) |
| 228 | + initialise_series = InitialiseDistroSeries(child) |
| 229 | + try: |
| 230 | + initialise_series.check() |
| 231 | + except InitialisationError, e: |
| 232 | + raise DerivationError(e) |
| 233 | + getUtility(IInitialiseDistroSeriesJobSource).create( |
| 234 | + child, architectures, packagesets, rebuild) |
| 235 | + |
| 236 | |
| 237 | class DistroSeriesSet: |
| 238 | implements(IDistroSeriesSet) |
| 239 | |
| 240 | === added file 'lib/lp/registry/stories/webservice/xx-derivedistroseries.txt' |
| 241 | --- lib/lp/registry/stories/webservice/xx-derivedistroseries.txt 1970-01-01 00:00:00 +0000 |
| 242 | +++ lib/lp/registry/stories/webservice/xx-derivedistroseries.txt 2010-10-12 08:34:50 +0000 |
| 243 | @@ -0,0 +1,68 @@ |
| 244 | +Derive Distributions |
| 245 | +-------------------- |
| 246 | + |
| 247 | +Using the DistroSeries.deriveDistroSeries() function, we can call it with the |
| 248 | +parent distroseries. We can call it with the distroseries already created, |
| 249 | +or it can create it for the user. |
| 250 | + |
| 251 | +Set Up |
| 252 | +------ |
| 253 | + |
| 254 | + >>> login('admin@canonical.com') |
| 255 | + >>> soyuz = factory.makeTeam(name='soyuz-team') |
| 256 | + >>> parent = factory.makeDistroSeries() |
| 257 | + >>> child = factory.makeDistroSeries(parent_series=parent) |
| 258 | + >>> other = factory.makeDistroSeries() |
| 259 | + >>> logout() |
| 260 | + >>> from canonical.launchpad.testing.pages import webservice_for_person |
| 261 | + >>> from canonical.launchpad.webapp.interfaces import OAuthPermission |
| 262 | + >>> soyuz_webservice = webservice_for_person( |
| 263 | + ... soyuz.teamowner, permission=OAuthPermission.WRITE_PUBLIC) |
| 264 | + |
| 265 | +Calling |
| 266 | +------- |
| 267 | + |
| 268 | +We can't call .deriveDistroSeries() with a distroseries that isn't the |
| 269 | +child's parent |
| 270 | + |
| 271 | + >>> series_url = '/%s/%s' % (parent.parent.name, parent.name) |
| 272 | + >>> other_series_url = '/%s/%s' % ( |
| 273 | + ... other.parent.name, other.name) |
| 274 | + >>> child_name = child.name |
| 275 | + >>> series = webservice.get(series_url).jsonBody() |
| 276 | + >>> other_series = webservice.get(other_series_url).jsonBody() |
| 277 | + >>> derived = soyuz_webservice.named_post( |
| 278 | + ... other_series['self_link'], 'deriveDistroSeries', {}, |
| 279 | + ... name=child_name, rebuild=False) |
| 280 | + >>> print derived |
| 281 | + HTTP/1.1 400 Bad Request |
| 282 | + Status: 400 Bad Request |
| 283 | + ... |
| 284 | + <BLANKLINE> |
| 285 | + DistroSeries ... parent series isn't ... |
| 286 | + <BLANKLINE> |
| 287 | + ... |
| 288 | + |
| 289 | +If we call it correctly, it works. |
| 290 | + |
| 291 | + >>> derived = soyuz_webservice.named_post( |
| 292 | + ... series['self_link'], 'deriveDistroSeries', {}, |
| 293 | + ... name=child_name, rebuild=False) |
| 294 | + >>> print derived |
| 295 | + HTTP/1.1 200 Ok |
| 296 | + Status: 200 Ok |
| 297 | + ... |
| 298 | + <BLANKLINE> |
| 299 | + ... |
| 300 | + |
| 301 | +And we can verify the job exists. |
| 302 | + |
| 303 | + >>> from zope.component import getUtility |
| 304 | + >>> from lp.soyuz.interfaces.distributionjob import ( |
| 305 | + ... IInitialiseDistroSeriesJobSource) |
| 306 | + >>> login('admin@canonical.com') |
| 307 | + >>> [job] = list( |
| 308 | + ... getUtility(IInitialiseDistroSeriesJobSource).iterReady()) |
| 309 | + >>> job.distroseries == child |
| 310 | + True |
| 311 | + |
| 312 | |
| 313 | === added file 'lib/lp/registry/tests/test_derivedistroseries.py' |
| 314 | --- lib/lp/registry/tests/test_derivedistroseries.py 1970-01-01 00:00:00 +0000 |
| 315 | +++ lib/lp/registry/tests/test_derivedistroseries.py 2010-10-12 08:34:50 +0000 |
| 316 | @@ -0,0 +1,78 @@ |
| 317 | +# Copyright 2010 Canonical Ltd. This software is licensed under the |
| 318 | +# GNU Affero General Public License version 3 (see the file LICENSE). |
| 319 | + |
| 320 | +"""Test initialising a distroseries using |
| 321 | +IDistroSeries.deriveDistroSeries.""" |
| 322 | + |
| 323 | +__metaclass__ = type |
| 324 | + |
| 325 | +from canonical.testing.layers import LaunchpadFunctionalLayer |
| 326 | +from lp.registry.interfaces.distroseries import DerivationError |
| 327 | +from lp.soyuz.interfaces.distributionjob import ( |
| 328 | + IInitialiseDistroSeriesJobSource, |
| 329 | + ) |
| 330 | +from lp.testing import ( |
| 331 | + login, |
| 332 | + logout, |
| 333 | + TestCaseWithFactory, |
| 334 | + ) |
| 335 | +from zope.component import getUtility |
| 336 | +from zope.security.interfaces import Unauthorized |
| 337 | + |
| 338 | + |
| 339 | +class TestDeriveDistroSeries(TestCaseWithFactory): |
| 340 | + |
| 341 | + layer = LaunchpadFunctionalLayer |
| 342 | + |
| 343 | + def setUp(self): |
| 344 | + super(TestDeriveDistroSeries, self).setUp() |
| 345 | + self.soyuz = self.factory.makeTeam(name='soyuz-team') |
| 346 | + self.parent = self.factory.makeDistroSeries() |
| 347 | + self.child = self.factory.makeDistroSeries( |
| 348 | + parent_series=self.parent) |
| 349 | + |
| 350 | + def test_no_permission_to_call(self): |
| 351 | + login('admin@canonical.com') |
| 352 | + person = self.factory.makePerson() |
| 353 | + logout() |
| 354 | + self.assertRaises( |
| 355 | + Unauthorized, self.parent.deriveDistroSeries, person, |
| 356 | + self.child.name) |
| 357 | + |
| 358 | + def test_no_distroseries_and_no_arguments(self): |
| 359 | + """Test that calling deriveDistroSeries() when the distroseries |
| 360 | + doesn't exist, and not enough arguments are specified that the |
| 361 | + function errors.""" |
| 362 | + self.assertRaisesWithContent( |
| 363 | + DerivationError, |
| 364 | + 'Display Name, Title, Summary, Description and Version all ' |
| 365 | + 'need to be set when creating a distroseries', |
| 366 | + self.parent.deriveDistroSeries, self.soyuz.teamowner, |
| 367 | + 'foobuntu') |
| 368 | + |
| 369 | + def test_parent_is_not_self(self): |
| 370 | + login('admin@canonical.com') |
| 371 | + other = self.factory.makeDistroSeries() |
| 372 | + logout() |
| 373 | + self.assertRaisesWithContent( |
| 374 | + DerivationError, |
| 375 | + "DistroSeries %s parent series isn't %s" % ( |
| 376 | + self.child.name, other.name), |
| 377 | + other.deriveDistroSeries, self.soyuz.teamowner, |
| 378 | + self.child.name) |
| 379 | + |
| 380 | + def test_create_new_distroseries(self): |
| 381 | + self.parent.deriveDistroSeries( |
| 382 | + self.soyuz.teamowner, self.child.name) |
| 383 | + [job] = list( |
| 384 | + getUtility(IInitialiseDistroSeriesJobSource).iterReady()) |
| 385 | + self.assertEqual(job.distroseries, self.child) |
| 386 | + |
| 387 | + def test_create_fully_new_distroseries(self): |
| 388 | + self.parent.deriveDistroSeries( |
| 389 | + self.soyuz.teamowner, 'foobuntu', displayname='Foobuntu', |
| 390 | + title='The Foobuntu', summary='Foobuntu', |
| 391 | + description='Foobuntu is great', version='11.11') |
| 392 | + [job] = list( |
| 393 | + getUtility(IInitialiseDistroSeriesJobSource).iterReady()) |
| 394 | + self.assertEqual(job.distroseries.name, 'foobuntu') |
| 395 | |
| 396 | === modified file 'lib/lp/soyuz/configure.zcml' |
| 397 | --- lib/lp/soyuz/configure.zcml 2010-10-06 18:53:53 +0000 |
| 398 | +++ lib/lp/soyuz/configure.zcml 2010-10-12 08:34:50 +0000 |
| 399 | @@ -905,9 +905,12 @@ |
| 400 | provides="lp.soyuz.interfaces.distributionjob.IInitialiseDistroSeriesJobSource"> |
| 401 | <allow interface="lp.soyuz.interfaces.distributionjob.IInitialiseDistroSeriesJobSource"/> |
| 402 | </securedutility> |
| 403 | - |
| 404 | + <class class="lp.soyuz.model.distributionjob.DistributionJob"> |
| 405 | + <allow interface="lp.soyuz.interfaces.distributionjob.IDistributionJob" /> |
| 406 | + </class> |
| 407 | <class class="lp.soyuz.model.initialisedistroseriesjob.InitialiseDistroSeriesJob"> |
| 408 | - <allow interface="lp.services.job.interfaces.job.IRunnableJob" /> |
| 409 | + <allow interface="lp.soyuz.interfaces.distributionjob.IInitialiseDistroSeriesJob" /> |
| 410 | + <allow interface="lp.soyuz.interfaces.distributionjob.IDistributionJob" /> |
| 411 | </class> |
| 412 | |
| 413 | </configure> |
| 414 | |
| 415 | === modified file 'lib/lp/soyuz/interfaces/distributionjob.py' |
| 416 | --- lib/lp/soyuz/interfaces/distributionjob.py 2010-09-16 01:38:42 +0000 |
| 417 | +++ lib/lp/soyuz/interfaces/distributionjob.py 2010-10-12 08:34:50 +0000 |
| 418 | @@ -58,7 +58,7 @@ |
| 419 | class IInitialiseDistroSeriesJobSource(IJobSource): |
| 420 | """An interface for acquiring IDistributionJobs.""" |
| 421 | |
| 422 | - def create(distroseries): |
| 423 | + def create(distroseries, arches, packagesets, rebuild): |
| 424 | """Create a new initialisation job for a distroseries.""" |
| 425 | |
| 426 | |
| 427 | |
| 428 | === modified file 'lib/lp/soyuz/interfaces/packagecloner.py' |
| 429 | --- lib/lp/soyuz/interfaces/packagecloner.py 2010-10-01 17:48:37 +0000 |
| 430 | +++ lib/lp/soyuz/interfaces/packagecloner.py 2010-10-12 08:34:50 +0000 |
| 431 | @@ -19,7 +19,8 @@ |
| 432 | |
| 433 | def clonePackages( |
| 434 | origin, destination, distroarchseries_list=None, |
| 435 | - proc_familes=None, always_create=False): |
| 436 | + proc_families=None, sourcepackagenames=None, |
| 437 | + always_create=False): |
| 438 | """Copies the source packages from origin to destination as |
| 439 | well as the binary packages for the DistroArchSeries specified. |
| 440 | |
| 441 | @@ -27,8 +28,10 @@ |
| 442 | :param destination: the location to which the data is to be copied. |
| 443 | :param distroarchseries_list: the binary packages will be copied |
| 444 | for the distroarchseries pairs specified (if any). |
| 445 | - :param proc_familes: the processor families that builds will be |
| 446 | + :param proc_families: the processor families that builds will be |
| 447 | created for. |
| 448 | + :param sourcepackagenames: the source packages which are to be |
| 449 | + copied. |
| 450 | :param always_create: if builds should always be created. |
| 451 | """ |
| 452 | |
| 453 | |
| 454 | === modified file 'lib/lp/soyuz/model/initialisedistroseriesjob.py' |
| 455 | --- lib/lp/soyuz/model/initialisedistroseriesjob.py 2010-09-16 01:38:42 +0000 |
| 456 | +++ lib/lp/soyuz/model/initialisedistroseriesjob.py 2010-10-12 08:34:50 +0000 |
| 457 | @@ -33,16 +33,34 @@ |
| 458 | classProvides(IInitialiseDistroSeriesJobSource) |
| 459 | |
| 460 | @classmethod |
| 461 | - def create(cls, distroseries): |
| 462 | + def create( |
| 463 | + cls, distroseries, arches=(), packagesets=(), rebuild=False): |
| 464 | """See `IInitialiseDistroSeriesJob`.""" |
| 465 | + metadata = { |
| 466 | + 'arches': arches, 'packagesets': packagesets, |
| 467 | + 'rebuild': rebuild} |
| 468 | job = DistributionJob( |
| 469 | distroseries.distribution, distroseries, cls.class_job_type, |
| 470 | - ()) |
| 471 | + metadata) |
| 472 | IMasterStore(DistributionJob).add(job) |
| 473 | return cls(job) |
| 474 | |
| 475 | + @property |
| 476 | + def arches(self): |
| 477 | + return tuple(self.metadata['arches']) |
| 478 | + |
| 479 | + @property |
| 480 | + def packagesets(self): |
| 481 | + return tuple(self.metadata['packagesets']) |
| 482 | + |
| 483 | + @property |
| 484 | + def rebuild(self): |
| 485 | + return self.metadata['rebuild'] |
| 486 | + |
| 487 | def run(self): |
| 488 | """See `IRunnableJob`.""" |
| 489 | - ids = InitialiseDistroSeries(self.distroseries) |
| 490 | + ids = InitialiseDistroSeries( |
| 491 | + self.distroseries, self.arches, self.packagesets, |
| 492 | + self.rebuild) |
| 493 | ids.check() |
| 494 | ids.initialise() |
| 495 | |
| 496 | === modified file 'lib/lp/soyuz/model/packagecloner.py' |
| 497 | --- lib/lp/soyuz/model/packagecloner.py 2010-10-05 05:26:58 +0000 |
| 498 | +++ lib/lp/soyuz/model/packagecloner.py 2010-10-12 08:34:50 +0000 |
| 499 | @@ -61,7 +61,8 @@ |
| 500 | implements(IPackageCloner) |
| 501 | |
| 502 | def clonePackages(self, origin, destination, distroarchseries_list=None, |
| 503 | - proc_families=None, always_create=False): |
| 504 | + proc_families=None, sourcepackagenames=None, |
| 505 | + always_create=False): |
| 506 | """Copies packages from origin to destination package location. |
| 507 | |
| 508 | Binary packages are only copied for the `DistroArchSeries` pairs |
| 509 | @@ -77,19 +78,24 @@ |
| 510 | for the distroarchseries pairs specified (if any). |
| 511 | @param proc_families: the processor families to create builds for. |
| 512 | @type proc_families: Iterable |
| 513 | + @param sourcepackagenames: the sourcepackages to copy to the |
| 514 | + destination |
| 515 | + @type sourcepackagenames: Iterable |
| 516 | @param always_create: if we should create builds for every source |
| 517 | package copied, useful if no binaries are to be copied. |
| 518 | @type always_create: Boolean |
| 519 | """ |
| 520 | # First clone the source packages. |
| 521 | - self._clone_source_packages(origin, destination) |
| 522 | + self._clone_source_packages( |
| 523 | + origin, destination, sourcepackagenames) |
| 524 | |
| 525 | # Are we also supposed to clone binary packages from origin to |
| 526 | # destination distroarchseries pairs? |
| 527 | if distroarchseries_list is not None: |
| 528 | for (origin_das, destination_das) in distroarchseries_list: |
| 529 | self._clone_binary_packages( |
| 530 | - origin, destination, origin_das, destination_das) |
| 531 | + origin, destination, origin_das, destination_das, |
| 532 | + sourcepackagenames) |
| 533 | |
| 534 | if proc_families is None: |
| 535 | proc_families = [] |
| 536 | @@ -147,8 +153,9 @@ |
| 537 | # Commit to avoid MemoryError: bug 304459 |
| 538 | transaction.commit() |
| 539 | |
| 540 | - def _clone_binary_packages(self, origin, destination, origin_das, |
| 541 | - destination_das): |
| 542 | + def _clone_binary_packages( |
| 543 | + self, origin, destination, origin_das, destination_das, |
| 544 | + sourcepackagenames=None): |
| 545 | """Copy binary publishing data from origin to destination. |
| 546 | |
| 547 | @type origin: PackageLocation |
| 548 | @@ -163,9 +170,12 @@ |
| 549 | @type destination_das: DistroArchSeries |
| 550 | @param destination_das: the DistroArchSeries to which to copy |
| 551 | binary packages |
| 552 | + @param sourcepackagenames: List of source packages to restrict |
| 553 | + the copy to |
| 554 | + @type sourcepackagenames: Iterable |
| 555 | """ |
| 556 | store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR) |
| 557 | - store.execute(''' |
| 558 | + query = ''' |
| 559 | INSERT INTO BinaryPackagePublishingHistory ( |
| 560 | binarypackagerelease, distroarchseries, status, |
| 561 | component, section, priority, archive, datecreated, |
| 562 | @@ -183,7 +193,22 @@ |
| 563 | destination.pocket, origin_das, |
| 564 | PackagePublishingStatus.PENDING, |
| 565 | PackagePublishingStatus.PUBLISHED, |
| 566 | - origin.pocket, origin.archive)) |
| 567 | + origin.pocket, origin.archive) |
| 568 | + |
| 569 | + if sourcepackagenames and len(sourcepackagenames) > 0: |
| 570 | + query += '''AND bpph.binarypackagerelease IN ( |
| 571 | + SELECT bpr.id |
| 572 | + FROM BinaryPackageRelease AS bpr, |
| 573 | + BinaryPackageBuild as bpb, |
| 574 | + SourcePackageRelease as spr, |
| 575 | + SourcePackageName as spn |
| 576 | + WHERE bpb.id = bpr.build AND |
| 577 | + bpb.source_package_release = spr.id |
| 578 | + AND spr.sourcepackagename = spn.id |
| 579 | + AND spn.name IN %s)''' % sqlvalues( |
| 580 | + sourcepackagenames) |
| 581 | + |
| 582 | + store.execute(query) |
| 583 | |
| 584 | def mergeCopy(self, origin, destination): |
| 585 | """Please see `IPackageCloner`.""" |
| 586 | @@ -378,7 +403,8 @@ |
| 587 | " AND secsrc.component = %s" % quote(destination.component)) |
| 588 | store.execute(pop_query) |
| 589 | |
| 590 | - def _clone_source_packages(self, origin, destination): |
| 591 | + def _clone_source_packages( |
| 592 | + self, origin, destination, sourcepackagenames): |
| 593 | """Copy source publishing data from origin to destination. |
| 594 | |
| 595 | @type origin: PackageLocation |
| 596 | @@ -387,6 +413,9 @@ |
| 597 | @type destination: PackageLocation |
| 598 | @param destination: the location to which the data is |
| 599 | to be copied. |
| 600 | + @type sourcepackagenames: Iterable |
| 601 | + @param sourcepackagenames: List of source packages to restrict |
| 602 | + the copy to |
| 603 | """ |
| 604 | store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR) |
| 605 | query = ''' |
| 606 | @@ -407,6 +436,15 @@ |
| 607 | PackagePublishingStatus.PUBLISHED, |
| 608 | origin.pocket, origin.archive) |
| 609 | |
| 610 | + if sourcepackagenames and len(sourcepackagenames) > 0: |
| 611 | + query += '''AND spph.sourcepackagerelease IN ( |
| 612 | + (SELECT spr.id |
| 613 | + FROM SourcePackageRelease AS spr, |
| 614 | + SourcePackageName AS spn |
| 615 | + WHERE spr.sourcepackagename = spn.id |
| 616 | + AND spn.name IN %s))''' % sqlvalues( |
| 617 | + sourcepackagenames) |
| 618 | + |
| 619 | if origin.packagesets: |
| 620 | query += '''AND spph.sourcepackagerelease IN |
| 621 | (SELECT spr.id |
| 622 | |
| 623 | === modified file 'lib/lp/soyuz/scripts/initialise_distroseries.py' |
| 624 | --- lib/lp/soyuz/scripts/initialise_distroseries.py 2010-10-06 11:46:51 +0000 |
| 625 | +++ lib/lp/soyuz/scripts/initialise_distroseries.py 2010-10-12 08:34:50 +0000 |
| 626 | @@ -16,7 +16,6 @@ |
| 627 | from canonical.launchpad.interfaces.lpstorm import IMasterStore |
| 628 | from lp.buildmaster.enums import BuildStatus |
| 629 | from lp.registry.interfaces.pocket import PackagePublishingPocket |
| 630 | -from lp.registry.model.distroseries import DistroSeries |
| 631 | from lp.soyuz.adapters.packagelocation import PackageLocation |
| 632 | from lp.soyuz.enums import ( |
| 633 | ArchivePurpose, |
| 634 | @@ -59,10 +58,14 @@ |
| 635 | in the initialisation of a derivative. |
| 636 | """ |
| 637 | |
| 638 | - def __init__(self, distroseries, arches=(), rebuild=False): |
| 639 | + def __init__( |
| 640 | + self, distroseries, arches=(), packagesets=(), rebuild=False): |
| 641 | + # Avoid circular imports |
| 642 | + from lp.registry.model.distroseries import DistroSeries |
| 643 | self.distroseries = distroseries |
| 644 | self.parent = self.distroseries.parent_series |
| 645 | self.arches = arches |
| 646 | + self.packagesets = packagesets |
| 647 | self.rebuild = rebuild |
| 648 | self._store = IMasterStore(DistroSeries) |
| 649 | |
| 650 | @@ -180,6 +183,15 @@ |
| 651 | """ |
| 652 | archive_set = getUtility(IArchiveSet) |
| 653 | |
| 654 | + spns = [] |
| 655 | + # The overhead from looking up each packageset is mitigated by |
| 656 | + # this usually running from a job |
| 657 | + if self.packagesets: |
| 658 | + for pkgsetname in self.packagesets: |
| 659 | + pkgset = getUtility(IPackagesetSet).getByName( |
| 660 | + pkgsetname, distroseries=self.parent) |
| 661 | + spns += list(pkgset.getSourcesIncluded()) |
| 662 | + |
| 663 | for archive in self.parent.distribution.all_distro_archives: |
| 664 | if archive.purpose not in ( |
| 665 | ArchivePurpose.PRIMARY, ArchivePurpose.DEBUG): |
| 666 | @@ -204,7 +216,7 @@ |
| 667 | distroarchseries_list = () |
| 668 | getUtility(IPackageCloner).clonePackages( |
| 669 | origin, destination, distroarchseries_list, |
| 670 | - proc_families, self.rebuild) |
| 671 | + proc_families, spns, self.rebuild) |
| 672 | |
| 673 | def _copy_component_section_and_format_selections(self): |
| 674 | """Copy the section, component and format selections from the parent |
| 675 | @@ -282,6 +294,8 @@ |
| 676 | parent_to_child = {} |
| 677 | # Create the packagesets, and any archivepermissions |
| 678 | for parent_ps in packagesets: |
| 679 | + if self.packagesets and parent_ps.name not in self.packagesets: |
| 680 | + continue |
| 681 | child_ps = getUtility(IPackagesetSet).new( |
| 682 | parent_ps.name, parent_ps.description, |
| 683 | self.distroseries.owner, distroseries=self.distroseries, |
| 684 | |
| 685 | === modified file 'lib/lp/soyuz/scripts/tests/test_initialise_distroseries.py' |
| 686 | --- lib/lp/soyuz/scripts/tests/test_initialise_distroseries.py 2010-10-06 11:46:51 +0000 |
| 687 | +++ lib/lp/soyuz/scripts/tests/test_initialise_distroseries.py 2010-10-12 08:34:50 +0000 |
| 688 | @@ -19,7 +19,10 @@ |
| 689 | from lp.registry.interfaces.pocket import PackagePublishingPocket |
| 690 | from lp.soyuz.enums import SourcePackageFormat |
| 691 | from lp.soyuz.interfaces.archivepermission import IArchivePermissionSet |
| 692 | -from lp.soyuz.interfaces.packageset import IPackagesetSet |
| 693 | +from lp.soyuz.interfaces.packageset import ( |
| 694 | + IPackagesetSet, |
| 695 | + NoSuchPackageSet, |
| 696 | + ) |
| 697 | from lp.soyuz.interfaces.publishing import PackagePublishingStatus |
| 698 | from lp.soyuz.interfaces.sourcepackageformat import ( |
| 699 | ISourcePackageFormatSelectionSet, |
| 700 | @@ -155,9 +158,9 @@ |
| 701 | child.isSourcePackageFormatPermitted( |
| 702 | SourcePackageFormat.FORMAT_1_0)) |
| 703 | |
| 704 | - def _full_initialise(self, arches=(), rebuild=False): |
| 705 | + def _full_initialise(self, arches=(), packagesets=(), rebuild=False): |
| 706 | child = self.factory.makeDistroSeries(parent_series=self.parent) |
| 707 | - ids = InitialiseDistroSeries(child, arches, rebuild) |
| 708 | + ids = InitialiseDistroSeries(child, arches, packagesets, rebuild) |
| 709 | ids.check() |
| 710 | ids.initialise() |
| 711 | return child |
| 712 | @@ -230,6 +233,33 @@ |
| 713 | child.main_archive, 'udev', uploader, |
| 714 | distroseries=child)) |
| 715 | |
| 716 | + def test_copy_limit_packagesets(self): |
| 717 | + # If a parent series has packagesets, we can decide which ones we |
| 718 | + # want to copy |
| 719 | + test1 = getUtility(IPackagesetSet).new( |
| 720 | + u'test1', u'test 1 packageset', self.parent.owner, |
| 721 | + distroseries=self.parent) |
| 722 | + test2 = getUtility(IPackagesetSet).new( |
| 723 | + u'test2', u'test 2 packageset', self.parent.owner, |
| 724 | + distroseries=self.parent) |
| 725 | + packages = ('udev', 'chromium', 'libc6') |
| 726 | + for pkg in packages: |
| 727 | + test1.addSources(pkg) |
| 728 | + child = self._full_initialise(packagesets=('test1',)) |
| 729 | + child_test1 = getUtility(IPackagesetSet).getByName( |
| 730 | + u'test1', distroseries=child) |
| 731 | + self.assertEqual(test1.description, child_test1.description) |
| 732 | + self.assertRaises( |
| 733 | + NoSuchPackageSet, getUtility(IPackagesetSet).getByName, |
| 734 | + u'test2', distroseries=child) |
| 735 | + parent_srcs = test1.getSourcesIncluded(direct_inclusion=True) |
| 736 | + child_srcs = child_test1.getSourcesIncluded( |
| 737 | + direct_inclusion=True) |
| 738 | + self.assertEqual(parent_srcs, child_srcs) |
| 739 | + child.updatePackageCount() |
| 740 | + self.assertEqual(child.sourcecount, len(packages)) |
| 741 | + self.assertEqual(child.binarycount, 2) # Chromium is FTBFS |
| 742 | + |
| 743 | def test_rebuild_flag(self): |
| 744 | # No binaries will get copied if we specify rebuild=True |
| 745 | self.parent.updatePackageCount() |
| 746 | @@ -254,6 +284,35 @@ |
| 747 | self.assertEqual( |
| 748 | das[0].architecturetag, self.parent_das.architecturetag) |
| 749 | |
| 750 | + def test_limit_packagesets_rebuild_and_one_das(self): |
| 751 | + # We can limit the source packages copied, and only builds |
| 752 | + # for the copied source will be created |
| 753 | + test1 = getUtility(IPackagesetSet).new( |
| 754 | + u'test1', u'test 1 packageset', self.parent.owner, |
| 755 | + distroseries=self.parent) |
| 756 | + test2 = getUtility(IPackagesetSet).new( |
| 757 | + u'test2', u'test 2 packageset', self.parent.owner, |
| 758 | + distroseries=self.parent) |
| 759 | + packages = ('udev', 'chromium') |
| 760 | + for pkg in packages: |
| 761 | + test1.addSources(pkg) |
| 762 | + self.factory.makeDistroArchSeries(distroseries=self.parent) |
| 763 | + child = self._full_initialise( |
| 764 | + arches=[self.parent_das.architecturetag], |
| 765 | + packagesets=('test1',), rebuild=True) |
| 766 | + child.updatePackageCount() |
| 767 | + builds = child.getBuildRecords( |
| 768 | + build_state=BuildStatus.NEEDSBUILD, |
| 769 | + pocket=PackagePublishingPocket.RELEASE) |
| 770 | + self.assertEqual(child.sourcecount, len(packages)) |
| 771 | + self.assertEqual(child.binarycount, 0) |
| 772 | + self.assertEqual(builds.count(), len(packages)) |
| 773 | + das = list(IStore(DistroArchSeries).find( |
| 774 | + DistroArchSeries, distroseries = child)) |
| 775 | + self.assertEqual(len(das), 1) |
| 776 | + self.assertEqual( |
| 777 | + das[0].architecturetag, self.parent_das.architecturetag) |
| 778 | + |
| 779 | def test_script(self): |
| 780 | # Do an end-to-end test using the command-line tool |
| 781 | uploader = self.factory.makePerson() |
| 782 | |
| 783 | === modified file 'lib/lp/soyuz/tests/test_initialisedistroseriesjob.py' |
| 784 | --- lib/lp/soyuz/tests/test_initialisedistroseriesjob.py 2010-09-16 01:38:42 +0000 |
| 785 | +++ lib/lp/soyuz/tests/test_initialisedistroseriesjob.py 2010-10-12 08:34:50 +0000 |
| 786 | @@ -72,3 +72,19 @@ |
| 787 | # returns an InitialisationError, then it's good. |
| 788 | self.assertRaisesWithContent( |
| 789 | InitialisationError, "Parent series required.", job.run) |
| 790 | + |
| 791 | + def test_arguments(self): |
| 792 | + """Test that InitialiseDistroSeriesJob specified with arguments can |
| 793 | + be gotten out again.""" |
| 794 | + distroseries = self.factory.makeDistroSeries() |
| 795 | + arches = (u'i386', u'amd64') |
| 796 | + packagesets = (u'foo', u'bar', u'baz') |
| 797 | + |
| 798 | + job = getUtility(IInitialiseDistroSeriesJobSource).create( |
| 799 | + distroseries, arches, packagesets) |
| 800 | + |
| 801 | + naked_job = removeSecurityProxy(job) |
| 802 | + self.assertEqual(naked_job.distroseries, distroseries) |
| 803 | + self.assertEqual(naked_job.arches, arches) |
| 804 | + self.assertEqual(naked_job.packagesets, packagesets) |
| 805 | + self.assertEqual(naked_job.rebuild, False) |
Hi Steven,
I don't really understand what a derived distro series is, but it
sounds good, and triggering a job to do it seems neat.
I have quite a few comments, some trivial, but some do need work. I'm
mostly concerned with the way authorization is handled and with test
coverage.
Gavin.
[1]
+ @operation_
+ name=TextLine(
+ title=_("The name of the distroseries to derive."),
+ required=True),
...
Here, and with the other parameters, consider using copy_field() to
copy the relevant fields from IDistroSeries instead of defining them
again. Some of them provide extra validation, and it ensures that
things remain in sync.
[2]
+ status=TextLine(
+ title=_("Status the derived distroseries to created is."),
s/created/create/
[3]
+ if not user.inTeam(
+ raise Unauthorized
Access should be configured in ZCML, see configure.zcml in
lib/lp/registry/.
You could:
- Write tests to ensure that only members of ~soyuz-team can access
deriveDistroS
- Define a new security adapter that only permits, say, launchpad.Edit
to ~soyuz-team,
- Define a new interface, IDistroSeriesDe
deriveDistroS
it.
- Add something like the following to the ZCML:
<require
[4]
+ #if self.distribution is ubuntu and not user.in_tech_board
+ #elsif not user a driver
I guess you can drop this.
[5]
+ for param in (displayname, summary, description, version):
+ if not param:
Avoid using implicit truthiness :) Or falsiness. Instead:
if param is None:
raise ...
Or, depending on what you need:
if param is None or len(param) == 0:
raise ...
[6]
Back to to the parameters:
+ arches=List(
+ title=_("The list of arches to copy to the derived "
+ "distroseries."),
+ required=False),
Is "arches" a standard term? If not, consider expanding this to
"architectures", or whatever it's meant to be, "archbishops" perhaps.
[7]
+ child = getUtility(
I'm probably missing something, but IDistroSeriesSet does not define a
new() method, and it's not implemented on DistroSeriesSet either. That
makes me suspect that this part of the conditional is not tested.
[8]
+ raise DerivationError(
+ "DistroSeries %s parent series isn't %s" % (
+ child.name, self.name))
Perhaps this exception could explain why this is a requirement too. Or
is it blindingly obvious to someone who expects to use this function.
[9]
+ ids = InitialiseDistr
Can you use a longer and more meaningful variable name?
[10]
+We can't call .deriveDistroSe
Trailing whitespace.