launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> === modified file 'lib/lp/snappy/interfaces/snap.py'
> --- lib/lp/snappy/interfaces/snap.py	2016-07-15 17:11:09 +0000
> +++ lib/lp/snappy/interfaces/snap.py	2016-07-18 17:19:30 +0000
> @@ -296,13 +309,26 @@
>          description=_(
>              "Pending builds of this snap package, sorted in descending "
>              "order of creation."),
> -        # Really ISnapBuild, patched in _schema_circular_imports.py.
> +        # Really ISnapBuild, patched in lp.snappy.interfaces.webservice.
>          value_type=Reference(schema=Interface), readonly=True)))
>  
>  
>  class ISnapEdit(IWebhookTarget):
>      """`ISnap` methods that require launchpad.Edit permission."""
>  
> +    # Really ISnapBuild, patched in lp.snappy.interfaces.webservice.
> +    @operation_returns_collection_of(Interface)
> +    @export_write_operation()
> +    @operation_for_version("devel")
> +    def requestAutoBuilds(logger=None):
> +        """Create and return automatic builds for this snap package.
> +
> +        :param logger: An optional logger.
> +        :raises CannotRequestAutoBuilds: if no auto_build_archive or
> +            auto_build_pocket is set.
> +        :return: A sequence of `ISnapBuild` instances.
> +        """

"requestAutoBuilds" is weird, since it's actually manually requesting what would normally be automatic builds. But the recipe equivalent is performDailyBuild, which doesn't make much sense here. Perhaps requestAutoBuilds is least bad.

> +
>      @export_destructor_operation()
>      @operation_for_version("devel")
>      def destroySelf():
> 
> === modified file 'lib/lp/snappy/model/snap.py'
> --- lib/lp/snappy/model/snap.py	2016-07-15 17:11:09 +0000
> +++ lib/lp/snappy/model/snap.py	2016-07-18 17:19:30 +0000
> @@ -380,6 +382,40 @@
>          result.order_by(order_by)
>          return result
>  
> +    def requestAutoBuilds(self, logger=None):
> +        """See `ISnapSet`."""
> +        builds = []
> +        if self.auto_build_archive is None:
> +            raise CannotRequestAutoBuilds("auto_build_archive")
> +        if self.auto_build_pocket is None:
> +            raise CannotRequestAutoBuilds("auto_build_pocket")
> +        self.is_stale = False
> +        if logger is not None:
> +            logger.debug(
> +                "Scheduling builds of snap package %s/%s",
> +                self.owner.name, self.name)
> +        for arch in self.getAllowedArchitectures():
> +            try:
> +                build = self.requestBuild(
> +                    self.owner, self.auto_build_archive, arch,
> +                    self.auto_build_pocket)
> +                if logger is not None:
> +                    logger.debug(
> +                        " - %s/%s/%s: Build requested.",
> +                        self.owner.name, self.name, arch.architecturetag)
> +                builds.append(build)
> +            except SnapBuildAlreadyPending as e:
> +                if logger is not None:
> +                    logger.warning(
> +                        " - %s/%s/%s: %s",
> +                        self.owner.name, self.name, arch.architecturetag, e)
> +            except Exception as e:
> +                if logger is not None:
> +                    logger.exception(
> +                        " - %s/%s/%s: %s",
> +                        self.owner.name, self.name, arch.architecturetag, e)

Does it make sense to ignore unknown exceptions when this is called through the API? I suspect that crashing would be better, since unlike the cronjob it won't cause possibly successful future work to be skipped.

> +        return builds
> +
>      def getBuildSummariesForSnapBuildIds(self, snap_build_ids):
>          """See `ISnap`."""
>          result = {}


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-export-request-auto-builds/+merge/300367
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.