launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve



Diff comments:

> 
> === 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-12 10:52:05 +0000
> @@ -234,7 +242,7 @@
>      @property
>      def error_messages(self):
>          """See `ISnapStoreUploadJob`."""
> -        return self.metadata.get("error_messages")
> +        return self.store_metadata.get("error_messages")

Is it worth fetching error_message, error_detail, and error_messages from the combined view when they can now only ever be written to the job-specific metadata?  It seems a bit confusing.

>  
>      @error_messages.setter
>      def error_messages(self, messages):
> 
> === 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-12 10:52:05 +0000
> @@ -641,3 +641,40 @@
>          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))
> +        self.assertIsNone(job.error_message)

That's odd; I was expecting a non-None error message at this point.  Can you double-check this, since it may mean the test isn't set up quite right?

> +
> +        # 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))
> +        self.assertIsNone(job.error_message)


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