Review: Needs Fixing code
Whew, here's a first pass.
94 + def content_type(self, path):
This isn't a legal method name, and there's no real reason for it to be a
method rather than a module-level function
105 + for dirpath, _, filenames in os.walk(self.upload_path):
Won't this blow up if my build returns a tree with the same filename in
multiple directories? Or do you just use walk to discover the only directory
that should exist?
329 + def get_segments(pillar_name):
330 + base = [self.context.name, pillar_name]
331 + return itertools.chain(iter(base), iter(self.request.stepstogo))
332 +
333 + pillar_name = self.request.stepstogo.next()
334 + livefs = getUtility(ILiveFSSet).traverse(get_segments(pillar_name))
This seems unnecessarily indirect, unreadable, and confusing. The only place I
can think of that needs this structure is branch traversal, where the path
length differs depending on whether it's a project or a distro branch, or an
alias. I'd do something more like ArchiveNavigation.traverse_binaryhits.
376 + distroseries=Reference(
377 + Interface, title=_("The owner of this live filesystem image.")),
distroseries is probably not the owner. Did you consider using copy_field, or
even the built-in copying functionality provided by export_factory_operation's
second argument?
378 + name=TextLine(
379 + title=_("The series for which the image should be built.")),
Hum.
423 + def createLiveFS(self, registrant, owner, distroseries, name,
metadata):
There's no permission check here.
470 +class ViewLiveFSBuild(DelegatedAuthorization):
471 + permission = 'launchpad.View'
472 + usedfor = ILiveFSBuild
473 +
474 + def iter_objects(self):
475 + yield self.obj.livefs
476 + yield self.obj.archive
This means that an archive can contain builds that the archive owner can't see
(if the LiveFS is owned by an invisible private team). That seems suboptimal.
516 + if sourcepackagename is None:
517 + ancestries = []
518 + else:
519 + ancestries = primary_archive.getPublishedSources(
520 + name=sourcepackagename,
521 + distroseries=distroseries, exact_match=True)
I'd rephrase this using ResultSet.first(), and doing a None check rather than
catching an IndexError.
666 + <require
667 + permission="launchpad.Edit"
668 + set_schema=".interfaces.livefs.ILiveFSEditableAttributes"
669 + set_attributes="date_last_modified"/>
Why is date_last_modified separate?
719 + <require
720 + permission="launchpad.Edit"
721 + set_schema=".interfaces.livefsbuild.ILiveFSFile"/>
The objects are immutable, so this probably shouldn't be a thing.
873 + metadata_override=Dict(
874 + title=_("A JSON string with a dict of data about the image."),
875 + key_type=TextLine(), required=False))
>From the perspective of the user of the interface, isn't this a real dict, not
>a JSON encoding of a dict?
877 + @export_factory_operation(Interface, [])
878 + @export_write_operation()
Both?
1147 + Call the can_be_cancelled() method prior to this one to find out if
1148 + cancelling the build is possible.
It's a property, not a method.
1380 + def __str__(self):
1381 + return '%s %s' % (self.distroseries, self.name)
This is needlessly ambiguous, and I hope nothing uses it anyway. Can it just be
dropped?
1419 + def requestBuild(self, requester, archive, distroarchseries, pocket,
1420 + unique_key=None, metadata_override=None):
No privilege check here either.
1478 + def exists(self, owner, distroseries, name):
1479 + """See `ILiveFSSet`."""
1480 + livefs = self.get(owner, distroseries, name)
1481 + if livefs:
1482 + return True
1483 + else:
1484 + return False
return livefs is not None?
1680 + dependencies = Unicode(name='dependencies')
This isn't in the interface or anywhere. Should it exist?
1700 + if metadata_override is None:
1701 + metadata_override = {}
You don't do the same with LiveFS.metadata. Would it be best to just leave it
as None here?
1731 + @property
1732 + def distro_series(self):
1733 + """See `IPackageBuild`."""
1734 + return self.distroarchseries.distroseries
We now have LiveFSBuild.distro_series and LiveFSBuild.distroarchseries.
1761 + # We provide this property for API convenience, but live filesystem
1762 + # builds cannot be retried. Request another build using
1763 + # LiveFS.requestBuild instead.
Why provide it? SPRBs and TTBJs don't. We can add it if we end up doing
retries, but I don't see any reason to suspect that we will.
1800 + def getMedianBuildDuration(self):
1801 + """Return the median duration of our successful builds."""
1802 + store = IStore(self)
1803 + result = store.find(
1804 + (LiveFSBuild.date_started, LiveFSBuild.date_finished),
1805 + LiveFSBuild.livefs == self.livefs_id,
1806 + LiveFSBuild.distroarchseries == self.distroarchseries_id,
1807 + LiveFSBuild.date_finished != None,
1808 + LiveFSBuild.status == BuildStatus.FULLYBUILT)
date_finished should always be set if the status is FULLYBUILT. I'd probably
drop the date_finished check, and look for impact on the index you added for
this method.
1823 + def getFiles(self):
1824 + """See `ILiveFSBuild`."""
1825 + store = Store.of(self)
1826 + result = store.find(
1827 + (LiveFSFile, LibraryFileAlias, LibraryFileContent),
1828 + LiveFSFile.livefsbuild == self.id,
1829 + LibraryFileAlias.id == LiveFSFile.libraryfile_id,
1830 + LibraryFileContent.id == LibraryFileAlias.contentID)
1831 + return result.order_by(
1832 + [LibraryFileAlias.filename, LiveFSFile.id]).config(distinct=True)
Why is this distinct?
1852 + def addFile(self, lfa):
1853 + """See `ILiveFSBuild`."""
1854 + return LiveFSFile(livefsbuild=self, libraryfile=lfa)
I'm not quite sure how this actually gets added to the store. Perhaps via an FK
somehow.
1869 + def getUploader(self, changes):
1870 + """See `IPackageBuild`."""
1871 + return self.requester
Should this exist? There's no ACL check, and no changes file.
1910 + def getByID(self, build_id):
1911 + """See `ISpecificBuildFarmJobSource`."""
1912 + store = IMasterStore(LiveFSBuild)
1913 + return store.find(LiveFSBuild, LiveFSBuild.id == build_id).one()
Consider store.get(LiveFSBuild, build_id) to maximise cachability.
--
https://code.launchpad.net/~cjwatson/launchpad/livefs/+merge/217261
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
