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.
_______________________________________________
Mailing list: https://launchpad.net/~launchpad-reviewers
Post to : [email protected]
Unsubscribe : https://launchpad.net/~launchpad-reviewers
More help : https://help.launchpad.net/ListHelp
