launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> === modified file 'lib/lp/snappy/interfaces/snapbuild.py'
> --- lib/lp/snappy/interfaces/snapbuild.py	2018-10-09 10:35:44 +0000
> +++ lib/lp/snappy/interfaces/snapbuild.py	2018-12-10 17:26:07 +0000
> @@ -34,7 +34,10 @@
>      Reference,
>      )
>  from zope.component.interfaces import IObjectEvent
> -from zope.interface import Interface
> +from zope.interface import (
> +    Attribute,
> +    Interface

Trailing comma please (utilities/format-new-and-modified-imports may help, though check its output as it gets slightly confused by try/except in import blocks).

> +    )
>  from zope.schema import (
>      Bool,
>      Choice,
> 
> === 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-10 17:26:07 +0000
> @@ -175,9 +175,11 @@
>  
>      failure_count = Int(name='failure_count', allow_none=False)
>  
> +    store_upload_metadata = JSON('store_upload_json_data', allow_none=False)
> +
>      def __init__(self, build_farm_job, requester, snap, archive,
>                   distro_arch_series, pocket, channels, processor, virtualized,
> -                 date_created, build_request=None):
> +                 date_created, store_upload_metadata, build_request=None):

I'd make this be store_upload_metadata=None and move the {} default from SnapBuildSet.new to here.

>          """Construct a `SnapBuild`."""
>          super(SnapBuild, self).__init__()
>          self.build_farm_job = build_farm_job
> @@ -507,7 +510,8 @@
>  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,
> +            store_upload_metadata=None, build_request=None, ):

Weird trailing comma-space?

>          """See `ISnapBuildSet`."""
>          store = IMasterStore(SnapBuild)
>          build_farm_job = getUtility(IBuildFarmJobSource).new(
> 
> === 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-10 17:26:07 +0000
> @@ -212,54 +212,71 @@
>          return job
>  
>      @property
> +    def store_metadata(self):
> +        """See `ISnapStoreUploadJob`."""
> +        intermediate = {}
> +        intermediate.update(self.metadata)
> +        intermediate.update(self.snapbuild.store_upload_metadata)
> +        return intermediate
> +
> +    @property
>      def error_message(self):
>          """See `ISnapStoreUploadJob`."""
> -        return self.metadata.get("error_message")
> +        return self.store_metadata.get("error_message")
>  
>      @error_message.setter
>      def error_message(self, message):
>          """See `ISnapStoreUploadJob`."""
> -        self.metadata["error_message"] = message
> +        self.snapbuild.store_upload_metadata["error_message"] = message
>  
>      @property
>      def error_detail(self):
>          """See `ISnapStoreUploadJob`."""
> -        return self.metadata.get("error_detail")
> +        return self.store_metadata.get("error_detail")
>  
>      @error_detail.setter
>      def error_detail(self, detail):
>          """See `ISnapStoreUploadJob`."""
> -        self.metadata["error_detail"] = detail
> +        self.snapbuild.store_upload_metadata["error_detail"] = detail
>  
>      @property
>      def error_messages(self):
>          """See `ISnapStoreUploadJob`."""
> -        return self.metadata.get("error_messages")
> +        return self.store_metadata.get("error_messages")
>  
>      @error_messages.setter
>      def error_messages(self, messages):
>          """See `ISnapStoreUploadJob`."""
> -        self.metadata["error_messages"] = messages
> +        self.snapbuild.store_upload_metadata["error_messages"] = messages

I'd say that error_* should remain as properties of each individual job, while store_url, store_revision, and status_url are properties of the build.

>  
>      @property
>      def store_url(self):
>          """See `ISnapStoreUploadJob`."""
> -        return self.metadata.get("store_url")
> +        return self.store_metadata.get("store_url")
>  
>      @store_url.setter
>      def store_url(self, url):
>          """See `ISnapStoreUploadJob`."""
> -        self.metadata["store_url"] = url
> +        self.snapbuild.store_upload_metadata["store_url"] = url
>  
>      @property
>      def store_revision(self):
>          """See `ISnapStoreUploadJob`."""
> -        return self.metadata.get("store_revision")
> +        return self.store_metadata.get("store_revision")
>  
>      @store_revision.setter
>      def store_revision(self, revision):
>          """See `ISnapStoreUploadJob`."""
> -        self.metadata["store_revision"] = revision
> +        self.snapbuild.store_upload_metadata["store_revision"] = revision
> +
> +    @property
> +    def status_url(self):
> +        """See `ISnapStoreUploadJob`."""
> +        return self.store_metadata.get('status_url')
> +
> +    @status_url.setter
> +    def status_url(self, url):
> +        self.snapbuild.store_upload_metadata["status_url"] = url
>  
>      # Ideally we'd just override Job._set_status or similar, but
>      # lazr.delegates makes that difficult, so we use this to override all
> @@ -313,13 +330,13 @@
>          """See `IRunnableJob`."""
>          client = getUtility(ISnapStoreClient)
>          try:
> -            if "status_url" not in self.metadata:
> -                self.metadata["status_url"] = client.upload(self.snapbuild)
> +            if "status_url" not in self.store_metadata:
> +                self.status_url = client.upload(self.snapbuild)
>                  # We made progress, so reset attempt_count.
>                  self.attempt_count = 1
>              if self.store_url is None:
>                  self.store_url, self.store_revision = (
> -                    client.checkStatus(self.metadata["status_url"]))
> +                    client.checkStatus(self.store_metadata["status_url"]))

self.status_url?

>                  # We made progress, so reset attempt_count.
>                  self.attempt_count = 1
>              if self.snapbuild.snap.store_channels:
> 
> === modified file 'lib/lp/snappy/tests/test_snapbuildjob.py'
> --- lib/lp/snappy/tests/test_snapbuildjob.py	2018-08-03 13:53:20 +0000
> +++ lib/lp/snappy/tests/test_snapbuildjob.py	2018-12-10 17:26:07 +0000
> @@ -641,3 +641,38 @@
>          self.assertIsNone(job.error_message)
>          self.assertEqual([], pop_notifications())
>          self.assertEqual(JobStatus.COMPLETED, job.job.status)
> +
> +    def test_retry_after_upload_does_not_upload(self):
> +        # If the job has uploaded, but failed to release, it should
> +        # not attempt to upload again on the next run.
> +        self.useFixture(FakeLogger())
> +        snapbuild = self.makeSnapBuild(store_channels=["stable", "edge"])
> +        self.assertContentEqual([], snapbuild.store_upload_jobs)
> +        job = SnapStoreUploadJob.create(snapbuild)
> +        client = FakeSnapStoreClient()
> +        client.upload.result = self.status_url
> +        client.checkStatus.result = (self.store_url, 1)
> +        client.release.failure = UploadFailedResponse(
> +            "Proxy error", can_retry=True)
> +        self.useFixture(ZopeUtilityFixture(client, ISnapStoreClient))
> +        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
> +            JobRunner([job]).runAll()
> +
> +        previous_upload = client.upload.calls
> +        previous_checkStatus = client.checkStatus.calls
> +        len_previous_release = len(client.release.calls)
> +
> +        # Check we uploaded as expected
> +        self.assertEqual(self.store_url, job.store_url)
> +        self.assertEqual(1, job.store_revision)
> +        self.assertEqual(timedelta(seconds=60), job.retry_delay)
> +        self.assertEqual(1, len(client.upload.calls))

Maybe also check that job.error_message is not None here, and that it's None after the successful release below.

> +
> +        # Run the job again
> +        with dbuser(config.ISnapStoreUploadJobSource.dbuser):
> +            JobRunner([job]).runAll()
> +
> +        # We should not have called `upload`, but moved straight to `release`
> +        self.assertEqual(previous_upload, client.upload.calls)
> +        self.assertEqual(previous_checkStatus, client.checkStatus.calls)
> +        self.assertEqual(len_previous_release + 1, len(client.release.calls))
> 
> === modified file 'lib/lp/testing/factory.py'
> --- lib/lp/testing/factory.py	2018-10-15 14:44:25 +0000
> +++ lib/lp/testing/factory.py	2018-12-10 17:26:07 +0000
> @@ -4770,7 +4770,7 @@
>                        archive=None, distroarchseries=None, pocket=None,
>                        channels=None, date_created=DEFAULT,
>                        status=BuildStatus.NEEDSBUILD, builder=None,
> -                      duration=None, **kwargs):
> +                      duration=None, metadata=None, **kwargs):

Is this a leftover?  The new parameter isn't passed to anything.

>          """Make a new SnapBuild."""
>          if requester is None:
>              requester = self.makePerson()


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