launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

I think we need a specific integration test for the main workflow that's at issue here: that is, if a SnapBuildJob successfully pushes but then fails to release, then a retry attempt doesn't retry the push but does retry the release.

Diff comments:

> === modified file 'lib/lp/snappy/model/snapbuild.py'
> --- lib/lp/snappy/model/snapbuild.py	2018-10-09 10:35:44 +0000
> +++ lib/lp/snappy/model/snapbuild.py	2018-12-06 14:08:45 +0000
> @@ -175,9 +175,11 @@
>  
>      failure_count = Int(name='failure_count', allow_none=False)
>  
> +    metadata = JSON('store_upload_json_data', allow_none=False)

Please declare this in the ISnapBuildView interface too, similar to the declaration in ISnapBuildJob.

> +
>      def __init__(self, build_farm_job, requester, snap, archive,
>                   distro_arch_series, pocket, channels, processor, virtualized,
> -                 date_created, build_request=None):
> +                 date_created, metadata, build_request=None):
>          """Construct a `SnapBuild`."""
>          super(SnapBuild, self).__init__()
>          self.build_farm_job = build_farm_job
> @@ -507,8 +510,11 @@
>  class SnapBuildSet(SpecificBuildFarmJobSourceMixin):
>  
>      def new(self, requester, snap, archive, distro_arch_series, pocket,
> -            channels=None, date_created=DEFAULT, build_request=None):
> +            channels=None, date_created=DEFAULT,
> +            build_request=None, metadata=None):

Can you move metadata before build_request so that it isn't gratuitously different from the SnapBuild constructor?

>          """See `ISnapBuildSet`."""
> +        if not metadata:
> +            metadata = {}

Maybe just pass "metadata=metadata or {}" to SnapBuild() instead of this.

>          store = IMasterStore(SnapBuild)
>          build_farm_job = getUtility(IBuildFarmJobSource).new(
>              SnapBuild.job_type, BuildStatus.NEEDSBUILD, date_created, None,
> 
> === modified file 'lib/lp/snappy/model/snapbuildjob.py'
> --- lib/lp/snappy/model/snapbuildjob.py	2018-09-18 14:13:16 +0000
> +++ lib/lp/snappy/model/snapbuildjob.py	2018-12-06 14:08:45 +0000
> @@ -244,22 +244,22 @@
>      @property
>      def store_url(self):
>          """See `ISnapStoreUploadJob`."""
> -        return self.metadata.get("store_url")
> +        return self.snapbuild.metadata.get("store_url")

There are many existing SnapBuildJobs in the database, and these properties are accessed from browser code.  Unless we plan to do a somewhat complicated data migration (which I think is probably not worthwhile), then all the accesses to self.snapbuild.metadata in this class need to check both old and new locations, preferring the new location if it exists.  To avoid the compatibility code being too repetitive, it might be worth having a metadata property in this class that exposes a merged dict for reading and sends all writes to the SnapBuild.

>  
>      @store_url.setter
>      def store_url(self, url):
>          """See `ISnapStoreUploadJob`."""
> -        self.metadata["store_url"] = url
> +        self.snapbuild.metadata["store_url"] = url
>  
>      @property
>      def store_revision(self):
>          """See `ISnapStoreUploadJob`."""
> -        return self.metadata.get("store_revision")
> +        return self.snapbuild.metadata.get("store_revision")
>  
>      @store_revision.setter
>      def store_revision(self, revision):
>          """See `ISnapStoreUploadJob`."""
> -        self.metadata["store_revision"] = revision
> +        self.snapbuild.metadata["store_revision"] = revision
>  
>      # Ideally we'd just override Job._set_status or similar, but
>      # lazr.delegates makes that difficult, so we use this to override all


-- 
https://code.launchpad.net/~twom/launchpad/move-snap-building-metadata-to-snapbuild/+merge/360192
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.