launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Needs Fixing

I think in general this looks OK, but I want to see a bit more effort to go through and root out vestiges of the old workflow.

Diff comments:

> === modified file 'lib/lp/snappy/model/snapbuildjob.py'
> --- lib/lp/snappy/model/snapbuildjob.py	2018-12-18 18:10:39 +0000
> +++ lib/lp/snappy/model/snapbuildjob.py	2019-06-19 10:53:29 +0000
> @@ -350,7 +350,6 @@
>                      raise ManualReview(
>                          "Package held for manual review on the store; "
>                          "cannot release it automatically.")

Is there any reason to preserve the manual-review check?  The only reason it exists is because we have to check it before doing the release, which is now irrelevant if the store is going to be handling that workflow.  (If you agree and remove it, remember to also remove its associated exception handling and email code and templates.)

The checkStatus stuff before that is slightly less clear.  Although we no longer really have a functional need for it, we probably still want to keep it because it lets us provide links to the revision in the store, and also SnapBuild.store_upload_url and SnapBuild.store_upload_revision are exported on the API and people may well be using them.  However, I think we should do a couple of things:

 * remove the attempt_count increment after checkStatus, which is no longer needed if it's going to be the last thing that the job does
 * adjust SnapStoreClient.checkStatus to stop checking can_release (since we no longer care) and instead just return response_data["url"], response_data["revision"] as long as the upload has been scanned without errors

SnapBuildStoreUploadStatus.FAILEDTORELEASE should presumably now be impossible for new uploads, but we need to keep it for historical data.  However, it might be worth some kind of comment in the enum declaration explaining the situation.

> -                client.release(self.snapbuild, self.store_revision)

This means that ReleaseFailedResponse is no longer possible, so remove the exception handling for that and its associated email code and templates.

>              self.error_message = None
>          except self.retry_error_types:
>              raise
> 
> === modified file 'lib/lp/snappy/model/snapstoreclient.py'
> --- lib/lp/snappy/model/snapstoreclient.py	2019-06-14 14:46:15 +0000
> +++ lib/lp/snappy/model/snapstoreclient.py	2019-06-19 10:53:29 +0000
> @@ -266,6 +266,14 @@
>              "series": snap.store_series.name,
>              "built_at": snapbuild.date_started.isoformat(),
>              }
> +        # The security proxy is useless and breaks JSON serialisation.
> +        channels = removeSecurityProxy(snap.store_channels)
> +        if channels:
> +            # This will cause a release
> +            data.update({
> +                "channels": channels,
> +                "only_if_newer": True,
> +            })

Style: extra indent on the closing brace.

>          # XXX cjwatson 2016-05-09: This should add timeline information, but
>          # that's currently difficult in jobs.
>          try:
> 
> === modified file 'lib/lp/snappy/tests/test_snapstoreclient.py'
> --- lib/lp/snappy/tests/test_snapstoreclient.py	2019-06-14 14:49:25 +0000
> +++ lib/lp/snappy/tests/test_snapstoreclient.py	2019-06-19 10:53:29 +0000
> @@ -638,100 +673,3 @@
>          self.assertContentEqual([], responses.calls)
>          self.assertIsNone(
>              getUtility(IMemcacheClient).get(self.channels_memcache_key))
> -
> -    @responses.activate
> -    def test_release(self):
> -        snap = self.factory.makeSnap(
> -            store_upload=True,
> -            store_series=self.factory.makeSnappySeries(name="rolling"),
> -            store_name="test-snap",
> -            store_secrets=self._make_store_secrets(),
> -            store_channels=["stable", "edge"])
> -        snapbuild = self.factory.makeSnapBuild(snap=snap)
> -        self._addSnapReleaseResponse()

All the calls to _addSnapReleaseResponse are gone, so remove it too.  And it's probably worth doing a lint run since I'm fairly sure that ReleaseFailedResponse should now be an unused import.

> -        self.client.release(snapbuild, 1)
> -        self.assertThat(responses.calls[-1].request, RequestMatches(
> -            url=Equals("http://sca.example/dev/api/snap-release/";),
> -            method=Equals("POST"),
> -            headers=ContainsDict({"Content-Type": Equals("application/json")}),
> -            auth=("Macaroon", MacaroonsVerify(self.root_key)),
> -            json_data={
> -                "name": "test-snap", "revision": 1,
> -                "channels": ["stable", "edge"], "series": "rolling",
> -                }))
> -
> -    @responses.activate
> -    def test_release_no_discharge(self):
> -        root_key = hashlib.sha256(self.factory.getUniqueString()).hexdigest()
> -        root_macaroon = Macaroon(key=root_key)
> -        snap = self.factory.makeSnap(
> -            store_upload=True,
> -            store_series=self.factory.makeSnappySeries(name="rolling"),
> -            store_name="test-snap",
> -            store_secrets={"root": root_macaroon.serialize()},
> -            store_channels=["stable", "edge"])
> -        snapbuild = self.factory.makeSnapBuild(snap=snap)
> -        self._addSnapReleaseResponse()
> -        self.client.release(snapbuild, 1)
> -        self.assertThat(responses.calls[-1].request, RequestMatches(
> -            url=Equals("http://sca.example/dev/api/snap-release/";),
> -            method=Equals("POST"),
> -            headers=ContainsDict({"Content-Type": Equals("application/json")}),
> -            auth=("Macaroon", MacaroonsVerify(root_key)),
> -            json_data={
> -                "name": "test-snap", "revision": 1,
> -                "channels": ["stable", "edge"], "series": "rolling",
> -                }))
> -
> -    @responses.activate
> -    def test_release_needs_discharge_macaroon_refresh(self):
> -        store_secrets = self._make_store_secrets()
> -        snap = self.factory.makeSnap(
> -            store_upload=True,
> -            store_series=self.factory.makeSnappySeries(name="rolling"),
> -            store_name="test-snap", store_secrets=store_secrets,
> -            store_channels=["stable", "edge"])
> -        snapbuild = self.factory.makeSnapBuild(snap=snap)
> -        responses.add(
> -            "POST", "http://sca.example/dev/api/snap-release/";, status=401,
> -            headers={"WWW-Authenticate": "Macaroon needs_refresh=1"})
> -        self._addMacaroonRefreshResponse()
> -        self._addSnapReleaseResponse()
> -        self.client.release(snapbuild, 1)
> -        requests = [call.request for call in responses.calls]
> -        self.assertThat(requests, MatchesListwise([
> -            MatchesStructure.byEquality(path_url="/dev/api/snap-release/"),
> -            MatchesStructure.byEquality(path_url="/api/v2/tokens/refresh"),
> -            MatchesStructure.byEquality(path_url="/dev/api/snap-release/"),
> -            ]))
> -        self.assertNotEqual(
> -            store_secrets["discharge"], snap.store_secrets["discharge"])
> -
> -    @responses.activate
> -    def test_release_error(self):
> -        snap = self.factory.makeSnap(
> -            store_upload=True,
> -            store_series=self.factory.makeSnappySeries(name="rolling"),
> -            store_name="test-snap", store_secrets=self._make_store_secrets(),
> -            store_channels=["stable", "edge"])
> -        snapbuild = self.factory.makeSnapBuild(snap=snap)
> -        responses.add(
> -            "POST", "http://sca.example/dev/api/snap-release/";, status=503,
> -            json={"error_list": [{"message": "Failed to publish"}]})
> -        self.assertRaisesWithContent(
> -            ReleaseFailedResponse, "Failed to publish",
> -            self.client.release, snapbuild, 1)
> -
> -    @responses.activate
> -    def test_release_404(self):
> -        snap = self.factory.makeSnap(
> -            store_upload=True,
> -            store_series=self.factory.makeSnappySeries(name="rolling"),
> -            store_name="test-snap", store_secrets=self._make_store_secrets(),
> -            store_channels=["stable", "edge"])
> -        snapbuild = self.factory.makeSnapBuild(snap=snap)
> -        responses.add(
> -            "POST", "http://sca.example/dev/api/snap-release/";, status=404)
> -        self.assertRaisesWithContent(
> -            ReleaseFailedResponse, b"404 Client Error: Not Found",
> -            self.client.release, snapbuild, 1)


-- 
https://code.launchpad.net/~twom/launchpad/snap-release-intents/+merge/369028
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.