launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-auto-builds-oops into lp:launchpad.

Commit message:
Tolerate failures to get snapcraft.yaml in SnapSet.makeAutoBuilds.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1786832 in Launchpad itself: "Launchpad fails to trigger new snap builds on git changes"
  https://bugs.launchpad.net/launchpad/+bug/1786832

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-auto-builds-oops/+merge/353094

We already handled failures to request builds for each individual architecture, but not failure to work out which architectures to build.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-auto-builds-oops into lp:launchpad.

=== modified file 'lib/lp/snappy/model/snap.py'
--- lib/lp/snappy/model/snap.py	2018-08-06 14:42:30 +0000
+++ lib/lp/snappy/model/snap.py	2018-08-14 14:42:38 +0000
@@ -565,29 +565,36 @@
                              allow_failures=False, fetch_snapcraft_yaml=True,
                              logger=None):
         """See `ISnap`."""
-        if fetch_snapcraft_yaml:
-            try:
-                snapcraft_data = removeSecurityProxy(
-                    getUtility(ISnapSet).getSnapcraftYaml(self))
-            except CannotFetchSnapcraftYaml as e:
-                if not e.unsupported_remote:
-                    raise
-                # The only reason we can't fetch the file is because we
-                # don't support fetching from this repository's host.  In
-                # this case the best thing is to fall back to building for
-                # all supported architectures.
+        try:
+            if fetch_snapcraft_yaml:
+                try:
+                    snapcraft_data = removeSecurityProxy(
+                        getUtility(ISnapSet).getSnapcraftYaml(self))
+                except CannotFetchSnapcraftYaml as e:
+                    if not e.unsupported_remote:
+                        raise
+                    # The only reason we can't fetch the file is because we
+                    # don't support fetching from this repository's host.
+                    # In this case the best thing is to fall back to
+                    # building for all supported architectures.
+                    snapcraft_data = {}
+            else:
                 snapcraft_data = {}
-        else:
-            snapcraft_data = {}
-        # Sort by Processor.id for determinism.  This is chosen to be the
-        # same order as in BinaryPackageBuildSet.createForSource, to
-        # minimise confusion.
-        supported_arches = OrderedDict(
-            (das.architecturetag, das) for das in sorted(
-                self.getAllowedArchitectures(),
-                key=attrgetter("processor.id")))
-        architectures_to_build = determine_architectures_to_build(
-            snapcraft_data, supported_arches.keys())
+            # Sort by Processor.id for determinism.  This is chosen to be
+            # the same order as in BinaryPackageBuildSet.createForSource, to
+            # minimise confusion.
+            supported_arches = OrderedDict(
+                (das.architecturetag, das) for das in sorted(
+                    self.getAllowedArchitectures(),
+                    key=attrgetter("processor.id")))
+            architectures_to_build = determine_architectures_to_build(
+                snapcraft_data, supported_arches.keys())
+        except Exception as e:
+            if not allow_failures:
+                raise
+            elif logger is not None:
+                logger.exception(" - %s/%s: %s", self.owner.name, self.name, e)
+            return []
 
         builds = []
         for build_instance in architectures_to_build:

=== modified file 'lib/lp/snappy/tests/test_snap.py'
--- lib/lp/snappy/tests/test_snap.py	2018-08-06 14:42:30 +0000
+++ lib/lp/snappy/tests/test_snap.py	2018-08-14 14:42:38 +0000
@@ -1441,6 +1441,36 @@
         self.assertEqual(
             expected_log_entries, logger.getLogBuffer().splitlines())
 
+    def test_makeAutoBuilds_tolerates_failures(self):
+        # If requesting builds of one snap fails, ISnapSet.makeAutoBuilds
+        # just logs the problem and carries on to the next.
+        das1, snap1 = self.makeAutoBuildableSnap(is_stale=True)
+        das2, snap2 = self.makeAutoBuildableSnap(is_stale=True)
+        logger = BufferLogger()
+        self.useFixture(GitHostingFixture()).getBlob.failure = (
+            GitRepositoryBlobNotFound("dummy", "snap/snapcraft.yaml"))
+        self.assertEqual(
+            [], getUtility(ISnapSet).makeAutoBuilds(logger=logger))
+        # The u'...' here is a little odd, but that's how KeyError (and thus
+        # NotFoundError) stringifies.
+        expected_log_entries = [
+            "DEBUG Scheduling builds of snap package %s/%s" % (
+                snap1.owner.name, snap1.name),
+            "ERROR  - %s/%s: u'Cannot find snapcraft.yaml in %s'" % (
+                snap1.owner.name, snap1.name, snap1.git_ref.unique_name),
+            "DEBUG Scheduling builds of snap package %s/%s" % (
+                snap2.owner.name, snap2.name),
+            "ERROR  - %s/%s: u'Cannot find snapcraft.yaml in %s'" % (
+                snap2.owner.name, snap2.name, snap2.git_ref.unique_name),
+            ]
+        self.assertThat(
+            logger.getLogBuffer().splitlines(),
+            # Ignore ordering differences, since ISnapSet.makeAutoBuilds
+            # might process the two snaps in either order.
+            MatchesSetwise(*(Equals(entry) for entry in expected_log_entries)))
+        self.assertFalse(snap1.is_stale)
+        self.assertFalse(snap2.is_stale)
+
     def test_makeAutoBuilds_with_older_build(self):
         # If a previous build is not recent and the snap package is stale,
         # ISnapSet.makeAutoBuilds requests builds.