launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Resubmit

You should set a prerequisite branch if and only if you actually branched from the unmerged branch in question; otherwise you will get very confusing results.

However, you really will need to redo this on top of snap-store-add-edit-views, I'm afraid.  The reason is that Snap.name is unique per person, but the same name in snapcraft.yaml may be associated with a snap that exists in multiple series.  Defaulting name from what's in snapcraft.yaml will therefore fill in an initial form value that fails validation in some cases, and we shouldn't do that.  We must only use this to pick a default for Snap.store_name, not Snap.name.

Diff comments:

> === modified file 'lib/lp/code/interfaces/gitref.py'
> --- lib/lp/code/interfaces/gitref.py	2016-01-12 03:54:38 +0000
> +++ lib/lp/code/interfaces/gitref.py	2016-05-17 21:10:50 +0000
> @@ -314,6 +314,14 @@
>          "Whether there are recent changes in this repository that have not "
>          "yet been scanned.")
>  
> +    def getBlob(filename):
> +        """Get a blob by file name from the related Git repository at this
> +        specific Git revision.
> +
> +        :param filename: Relative path of a file in the repository.
> +        :return: A binary string with the blob content.
> +        """
> +

This method doesn't really pull its weight at the moment.  I'd drop it and just use self.context.repository.getBlob in the one caller.

While GitRef does have quite a few repository proxy attributes/methods at the moment, they're mostly for cases where it needs to present a similar interface to Branch in order that higher-level code can treat both Bazaar and Git branches similarly, and that consideration doesn't apply here.

>  
>  class IGitRefBatchNavigator(ITableBatchNavigator):
>      pass
> 
> === modified file 'lib/lp/snappy/browser/snap.py'
> --- lib/lp/snappy/browser/snap.py	2016-05-06 09:45:45 +0000
> +++ lib/lp/snappy/browser/snap.py	2016-05-17 21:10:50 +0000
> @@ -326,11 +329,31 @@
>  
>      @property
>      def initial_values(self):
> +        name = None
> +        if IGitRef.providedBy(self.context):
> +            # Try to extract Snap name from snapcraft.yaml file.
> +            try:
> +                blob = self.context.getBlob('snapcraft.yaml')
> +                # Beware of unsafe yaml.load()!
> +                name = yaml.safe_load(blob).get('name')
> +            except GitRepositoryScanFault:
> +                log.info("Failed to get Snap manifest from Git %s",
> +                          self.context.unique_name)
> +            except (KeyboardInterrupt, SystemExit):
> +                raise
> +            except:

Bare except is usually a red flag, and while it's OK in this specific case, it's a bit overcomplicated.  This would be more clearly written as:

  except GitRepositoryScanFault:
      log.info(...)
  except Exception:
      log.debug(...)

> +                log.debug("Failed to parse Snap manifest at %s",
> +                          self.context.unique_name)
> +            if name is None:
> +                self.widget_errors['name'] = (
> +                    "Failed to find a valid snapcraft.yaml file.")
> +
>          # XXX cjwatson 2015-09-18: Hack to ensure that we don't end up
>          # accidentally selecting ubuntu-rtm/14.09 or similar.
>          # ubuntu.currentseries will always be in BuildableDistroSeries.
>          series = getUtility(ILaunchpadCelebrities).ubuntu.currentseries
>          return {
> +            'name': name,
>              'owner': self.user,
>              'distro_series': series,
>              }
> 
> === modified file 'lib/lp/snappy/browser/tests/test_snap.py'
> --- lib/lp/snappy/browser/tests/test_snap.py	2016-05-06 09:45:45 +0000
> +++ lib/lp/snappy/browser/tests/test_snap.py	2016-05-17 21:10:50 +0000
> @@ -270,6 +274,41 @@
>              extract_text(find_tag_by_id(browser.contents, "privacy"))
>          )
>  
> +    def test_initial_name_extraction_git(self):
> +        [git_ref] = self.factory.makeGitRefs()
> +        git_ref.getBlob = FakeMethod(result='name: test-snap')
> +        view = create_initialized_view(git_ref, "+new-snap")
> +        initial_values = view.initial_values
> +        self.assertIn('name', initial_values)
> +        self.assertEqual('test-snap', initial_values['name'])
> +
> +    def test_initial_name_extraction_git_repo_error(self):
> +        [git_ref] = self.factory.makeGitRefs()
> +        git_ref.getBlob = FakeMethod(failure=GitRepositoryScanFault)
> +        view = create_initialized_view(git_ref, "+new-snap")
> +        initial_values = view.initial_values
> +        self.assertIn('name', initial_values)
> +        self.assertEqual(None, initial_values['name'])

assertIsNone is a better spelling (here and below).

> +
> +    def test_initial_name_extraction_git_invalid_data(self):
> +        for invalid_result in (None, 123, '', '[][]', '#name:test', ']'):
> +            [git_ref] = self.factory.makeGitRefs()
> +            git_ref.getBlob = FakeMethod(result=invalid_result)
> +            view = create_initialized_view(git_ref, "+new-snap")
> +            initial_values = view.initial_values
> +            self.assertIn('name', initial_values)
> +            self.assertEqual(None, initial_values['name'])
> +
> +    def test_initial_name_extraction_git_safe_yaml(self):
> +        [git_ref] = self.factory.makeGitRefs()
> +        git_ref.getBlob = FakeMethod(result='Malicious code encoded in YAML!')
> +        view = create_initialized_view(git_ref, "+new-snap")
> +        with mock.patch('yaml.load') as unsafe_load:
> +            with mock.patch('yaml.safe_load') as safe_load:
> +                initial_values = view.initial_values
> +        self.assertEqual(0, unsafe_load.call_count)
> +        self.assertEqual(1, safe_load.call_count)
> +
>  
>  class TestSnapAdminView(BrowserTestCase):
>  


-- 
https://code.launchpad.net/~maxiberta/launchpad/snap-name-extraction/+merge/294964
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.