launchpad-reviewers team mailing list archive

[Thomi Richards <thomi.richards@xxxxxxxxxxxxx>]

LGTM - one comment below for your consideration.

Diff comments:

> 
> === modified file 'lib/lp/snappy/model/snapstoreclient.py'
> --- lib/lp/snappy/model/snapstoreclient.py	2016-06-27 13:19:15 +0000
> +++ lib/lp/snappy/model/snapstoreclient.py	2016-06-30 16:26:55 +0000
> @@ -241,7 +274,88 @@
>                      error_message = "\n".join(
>                          error["message"] for error in response_data["errors"])
>                      raise ScanFailedResponse(error_message)
> +                elif not response_data["can_release"]:
> +                    return response_data["url"], None
>                  else:
> -                    return response_data["url"]
> +                    return response_data["url"], response_data["revision"]
>          except requests.HTTPError as e:
>              raise BadScanStatusResponse(e.args[0])
> +
> +    @classmethod
> +    def listChannels(cls):
> +        """See `ISnapStoreClient`."""
> +        if config.snappy.store_search_url is None:
> +            return _default_store_channels
> +        channels = None
> +        memcache_client = getUtility(IMemcacheClient)
> +        search_host = urlsplit(config.snappy.store_search_url).hostname
> +        memcache_key = ("%s:channels" % search_host).encode("UTF-8")
> +        cached_channels = memcache_client.get(memcache_key)
> +        if cached_channels is not None:
> +            try:
> +                channels = json.loads(cached_channels)
> +            except Exception:
> +                log.exception(
> +                    "Cannot load cached channels for %s; deleting" %
> +                    search_host)

I'm probably missing something here (I've not used the memcached client before). Is 'cached_channels' an object whose __str__ method does the actual retrieval, and can raise either an Exception instance, or an arbitrary subclass of Exception? 

My point is that if cached_channels is a string, then surely we should be catching json.JSONDecoreError instead of Exception?

> +                memcache_client.delete(memcache_key)
> +        if (channels is None and
> +                not getFeatureFlag(u"snap.disable_channel_search")):
> +            path = "api/v1/channels"
> +            timeline = cls._getTimeline()
> +            if timeline is not None:
> +                action = timeline.start("store-search-get", "/" + path)
> +            channels_url = urlappend(config.snappy.store_search_url, path)
> +            try:
> +                response = urlfetch(
> +                    channels_url, headers={"Accept": "application/hal+json"})
> +            except requests.HTTPError as e:
> +                raise BadSearchResponse(e.args[0])
> +            finally:
> +                if timeline is not None:
> +                    action.finish()
> +            channels = response.json().get("_embedded", {}).get(
> +                "clickindex:channel", [])
> +            expire_time = time.time() + 60 * 60 * 24
> +            memcache_client.set(
> +                memcache_key, json.dumps(channels), expire_time)
> +        if channels is None:
> +            channels = _default_store_channels
> +        return channels
> +
> +    @classmethod
> +    def release(cls, snapbuild, revision):
> +        """See `ISnapStoreClient`."""
> +        assert config.snappy.store_url is not None
> +        snap = snapbuild.snap
> +        assert snap.store_name is not None
> +        assert snap.store_series is not None
> +        assert snap.store_channels
> +        release_url = urlappend(
> +            config.snappy.store_url, "dev/api/snap-release/")
> +        data = {
> +            "name": snap.store_name,
> +            "revision": revision,
> +            # The security proxy is useless and breaks JSON serialisation.
> +            "channels": removeSecurityProxy(snap.store_channels),
> +            "series": snap.store_series.name,
> +            }
> +        # XXX cjwatson 2016-06-28: This should add timeline information, but
> +        # that's currently difficult in jobs.
> +        try:
> +            assert snap.store_secrets is not None
> +            urlfetch(
> +                release_url, method="POST", json=data,
> +                auth=MacaroonAuth(
> +                    snap.store_secrets["root"],
> +                    snap.store_secrets["discharge"]))
> +        except requests.HTTPError as e:
> +            if e.response is not None:
> +                error = None
> +                try:
> +                    error = e.response.json()["errors"]
> +                except Exception:
> +                    pass
> +                if error is not None:
> +                    raise ReleaseFailedResponse(error)
> +            raise BadReleaseResponse(e.args[0])


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-channels-store-client/+merge/298806
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-channels-store-client into lp:launchpad.