launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Thanks for this long review.  Most of these were of course my mistakes,
and where I haven't explicitly commented on something I've fixed it.

On Mon, May 05, 2014 at 11:55:05PM -0000, William Grant wrote:
> 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?

The latter.  I think the worst that will happen is that you get
confusing results because two of the LiveFSFiles attached to a
LiveFSBuild have the same name, but the slave won't return trees like
this anyway since it only looks through a single directory for output
files.

> 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.

Now that LiveFSBuilds are traversed through LiveFS rather than Archive,
as discussed on IRC, I think it's OK to keep this as it is, since the
sense in which an archive "contains" the builds in question is now
further diminished.

> 666	+ <require
> 667	+ permission="launchpad.Edit"
> 668	+ set_schema=".interfaces.livefs.ILiveFSEditableAttributes"
> 669	+ set_attributes="date_last_modified"/>
> 
> Why is date_last_modified separate?

My browser code (not yet committed) fails without this when trying to
edit a LiveFS, as follows:

Traceback (most recent call last):

    Module zope.publisher.publish, line 132, in publish
    result = publication.callObject(request, obj)
    Module lp.services.webapp.publication, line 464, in callObject
    return mapply(ob, request.getPositionalArguments(), request)
    Module zope.publisher.publish, line 107, in mapply
    return debug_call(obj, args)
    __traceback_info__: <security proxied zope.browserpage.metaconfigure.SimpleViewClass from /home/cjwatson/src/canonical/launchpad/lp-branches/livefs-browser/lib/lp/soyuz/browser/../../app/templates/generic-edit.pt instance at 0xf069fcc>
    Module zope.publisher.publish, line 113, in debug_call
    return obj(*args)
    Module lp.services.webapp.publisher, line 440, in __call__
    self.initialize()
    Module lp.app.browser.launchpadform, line 136, in initialize
    self.form_result = form_action.success(data)
    Module zope.formlib.form, line 620, in success
    return self.success_handler(self.form, self, data)
    Module lp.soyuz.browser.livefs, line 309, in request_action
    self.context, livefs_before_modification, field_names))
    Module zope.event, line 31, in notify
    subscriber(event)
    Module zope.component.event, line 24, in dispatch
    zope.component.subscribers(event, None)
    Module zope.component._api, line 136, in subscribers
    return sitemanager.subscribers(objects, interface)
    Module zope.component.registry, line 321, in subscribers
    return self.adapters.subscribers(objects, provided)
    Module zope.interface.adapter, line 583, in subscribers
    subscription(*objects)
    Module zope.component.event, line 32, in objectEventNotify
    zope.component.subscribers((event.object, event), None)
    Module zope.component._api, line 136, in subscribers
    return sitemanager.subscribers(objects, interface)
    Module zope.component.registry, line 321, in subscribers
    return self.adapters.subscribers(objects, provided)
    Module zope.interface.adapter, line 583, in subscribers
    subscription(*objects)
    Module lp.soyuz.model.livefs, line 66, in livefs_modified
    livefs.date_last_modified = UTC_NOW

ForbiddenAttribute: ('date_last_modified', <lp.soyuz.model.livefs.LiveFS object at 0xe51caac>)

> 1680	+ dependencies = Unicode(name='dependencies')
> 
> This isn't in the interface or anywhere. Should it exist?

This is a stub because it's in the IBuildFarmJob interface.

> 1731	+ @property
> 1732	+ def distro_series(self):
> 1733	+ """See `IPackageBuild`."""
> 1734	+ return self.distroarchseries.distroseries
> 
> We now have LiveFSBuild.distro_series and LiveFSBuild.distroarchseries.

The "distro_series" name comes from the IPackageBuild interface, which
I'm using basically because it has upload log handling.  What would you
suggest I do here?  Reimplement upload log handling and inherit directly
from IBuildFarmJob, perhaps?

> 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.

It annoys me when I have to do hasattr(build, "can_be_retried") in API
scripts, but I suppose that's my problem.  Removed.

> 1869	+ def getUploader(self, changes):
> 1870	+ """See `IPackageBuild`."""
> 1871	+ return self.requester
> 
> Should this exist? There's no ACL check, and no changes file.

It's in the IPackageBuild interface; but on reflection it's fine to
leave it at the default implementation from PackageBuildMixin which
raises NotImplementedError.  Removed.

-- 
https://code.launchpad.net/~cjwatson/launchpad/livefs/+merge/217261
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.