launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code



Diff comments:

> 
> === modified file 'lib/lp/code/model/branch.py'
> --- lib/lp/code/model/branch.py	2018-06-14 17:09:31 +0000
> +++ lib/lp/code/model/branch.py	2018-06-14 17:09:31 +0000
> @@ -783,6 +789,63 @@
>                  RevisionAuthor, revisions, ['revision_author_id'])
>          return DecoratedResultSet(result, pre_iter_hook=eager_load)
>  
> +    def getBlob(self, filename, revision_id=None, enable_memcache=None,
> +                logger=None):
> +        """See `IBranch`."""
> +        hosting_client = getUtility(IBranchHostingClient)
> +        if enable_memcache is None:
> +            enable_memcache = not getFeatureFlag(
> +                u'code.bzr.blob.disable_memcache')
> +        if revision_id is None:
> +            revision_id = self.last_scanned_id
> +        if revision_id is None:
> +            # revision_id may still be None if the branch scanner hasn't
> +            # scanned this branch yet.  In this case, we won't be able to
> +            # guarantee that subsequent calls to this method within the same
> +            # transaction with revision_id=None will see the same revision,
> +            # and we won't be able to cache file lists.  Neither is fatal,
> +            # and this should be relatively rare.
> +            enable_memcache = False
> +        dirname = os.path.dirname(filename)
> +        unset = object()
> +        file_list = unset

Can you just use None here, since it's otherwise unused?

> +        if enable_memcache:
> +            memcache_client = getUtility(IMemcacheClient)
> +            instance_name = urlsplit(
> +                config.codehosting.internal_bzr_api_endpoint).hostname
> +            memcache_key = '%s:bzr-file-list:%s:%s:%s' % (
> +                instance_name, self.id, revision_id, dirname)
> +            if not isinstance(memcache_key, bytes):
> +                memcache_key = memcache_key.encode('UTF-8')
> +            cached_file_list = memcache_client.get(memcache_key)
> +            if cached_file_list is not None:
> +                try:
> +                    file_list = json.loads(cached_file_list)
> +                except Exception:
> +                    logger.exception(
> +                        'Cannot load cached file list for %s:%s:%s; deleting' %
> +                        (self.unique_name, revision_id, dirname))
> +                    memcache_client.delete(memcache_key)
> +        if file_list is unset:
> +            try:
> +                inventory = hosting_client.getInventory(
> +                    self.unique_name, dirname, rev=revision_id)
> +                file_list = {
> +                    entry['filename']: entry['file_id']
> +                    for entry in inventory['filelist']}
> +            except BranchFileNotFound:
> +                file_list = None
> +            if enable_memcache:
> +                # Cache the file list in case there's a request for another
> +                # file in the same directory.
> +                memcache_client.set(memcache_key, json.dumps(file_list))
> +        file_id = (file_list or {}).get(os.path.basename(filename))
> +        if file_id is None:
> +            raise BranchFileNotFound(
> +                self.unique_name, filename=filename, rev=revision_id)
> +        return hosting_client.getBlob(
> +            self.unique_name, file_id, rev=revision_id)
> +
>      def getDiff(self, new, old=None):
>          """See `IBranch`."""
>          hosting_client = getUtility(IBranchHostingClient)
> 
> === modified file 'lib/lp/snappy/browser/snap.py'
> --- lib/lp/snappy/browser/snap.py	2018-04-21 10:01:22 +0000
> +++ lib/lp/snappy/browser/snap.py	2018-06-14 17:09:31 +0000
> @@ -424,34 +422,19 @@
>      @property
>      def initial_values(self):
>          store_name = None
> -        if self.has_snappy_distro_series and IGitRef.providedBy(self.context):
> +        if self.has_snappy_distro_series:
>              # Try to extract Snap store name from snapcraft.yaml file.
>              try:
> -                paths = (
> -                    'snap/snapcraft.yaml',
> -                    'snapcraft.yaml',
> -                    '.snapcraft.yaml',
> -                    )
> -                for i, path in enumerate(paths):
> -                    try:
> -                        blob = self.context.repository.getBlob(
> -                            path, self.context.name)
> -                        break
> -                    except GitRepositoryBlobNotFound:
> -                        if i == len(paths) - 1:
> -                            raise
> -                # Beware of unsafe yaml.load()!
> -                store_name = yaml.safe_load(blob).get('name')
> -            except GitRepositoryScanFault:
> -                log.exception("Failed to get Snap manifest from Git %s",
> -                              self.context.unique_name)
> -            except (AttributeError, yaml.YAMLError):
> -                # Ignore parsing errors from invalid, user-supplied YAML
> +                snapcraft_data = getUtility(ISnapSet).getSnapcraftYaml(
> +                    self.context, logger=log)
> +            except (NotFoundError, CannotFetchSnapcraftYaml,
> +                    CannotParseSnapcraftYaml):
>                  pass
> -            except Exception as e:
> -                log.exception(
> -                    "Failed to extract name from Snap manifest at Git %s: %s",
> -                    self.context.unique_name, unicode(e))
> +            else:
> +                try:
> +                    store_name = snapcraft_data.get('name')
> +                except AttributeError:

Can this ever be raised, since getSnapcraftYaml ensures the top level is a dict?

> +                    pass
>  
>          store_series = getUtility(ISnappySeriesSet).getAll().first()
>          if store_series.preferred_distro_series is not None:


-- 
https://code.launchpad.net/~cjwatson/launchpad/snap-initial-name-bzr/+merge/345757
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.