launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

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.