launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/launchpad:snap-improve-logging into launchpad:master.

Commit message:
Improve logging of snap store upload scheduling

It's helpful to log the decision in snap_build_status_changed on whether
to schedule a store upload, since otherwise things are a bit invisible
when debugging user-reported problems.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/373915
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/launchpad:snap-improve-logging into launchpad:master.

diff --git a/lib/lp/snappy/model/snapbuild.py b/lib/lp/snappy/model/snapbuild.py
index 0415c66..6971e83 100644
--- a/lib/lp/snappy/model/snapbuild.py
+++ b/lib/lp/snappy/model/snapbuild.py
@@ -222,6 +222,10 @@ class SnapBuild(PackageBuildMixin, Storm):
             self.archive.private
         )
 
+    def __repr__(self):
+        return "<SnapBuild ~%s/+snap/%s/+build/%d>" % (
+            self.snap.owner.name, self.snap.name, self.id)
+
     @property
     def title(self):
         das = self.distro_arch_series
diff --git a/lib/lp/snappy/subscribers/snapbuild.py b/lib/lp/snappy/subscribers/snapbuild.py
index 86196cd..0eaa0dc 100644
--- a/lib/lp/snappy/subscribers/snapbuild.py
+++ b/lib/lp/snappy/subscribers/snapbuild.py
@@ -1,4 +1,4 @@
-# Copyright 2016-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2016-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Event subscribers for snap builds."""
@@ -11,6 +11,7 @@ from zope.component import getUtility
 
 from lp.buildmaster.enums import BuildStatus
 from lp.services.features import getFeatureFlag
+from lp.services.scripts import log
 from lp.services.webapp.publisher import canonical_url
 from lp.services.webhooks.interfaces import IWebhookSet
 from lp.services.webhooks.payload import compose_webhook_payload
@@ -41,9 +42,14 @@ def snap_build_status_changed(snapbuild, event):
     """Trigger events when snap package build statuses change."""
     _trigger_snap_build_webhook(snapbuild, "status-changed")
 
-    if (snapbuild.snap.can_upload_to_store and snapbuild.snap.store_upload and
-            snapbuild.status == BuildStatus.FULLYBUILT):
-        getUtility(ISnapStoreUploadJobSource).create(snapbuild)
+    if snapbuild.status == BuildStatus.FULLYBUILT:
+        if snapbuild.snap.can_upload_to_store and snapbuild.snap.store_upload:
+            log.info("Scheduling upload of %r to the store." % snapbuild)
+            getUtility(ISnapStoreUploadJobSource).create(snapbuild)
+        else:
+            log.info(
+                "%r is not configured for upload to the store." %
+                snapbuild.snap)
 
 
 def snap_build_store_upload_status_changed(snapbuild, event):
diff --git a/lib/lp/snappy/tests/test_snapbuild.py b/lib/lp/snappy/tests/test_snapbuild.py
index 1c28c20..aa69ae3 100644
--- a/lib/lp/snappy/tests/test_snapbuild.py
+++ b/lib/lp/snappy/tests/test_snapbuild.py
@@ -113,6 +113,14 @@ class TestSnapBuild(TestCaseWithFactory):
         self.assertProvides(self.build, IPackageBuild)
         self.assertProvides(self.build, ISnapBuild)
 
+    def test___repr__(self):
+        # SnapBuild has an informative __repr__.
+        self.assertEqual(
+            "<SnapBuild ~%s/+snap/%s/+build/%s>" % (
+                self.build.snap.owner.name, self.build.snap.name,
+                self.build.id),
+            repr(self.build))
+
     def test_title(self):
         # SnapBuild has an informative title.
         das = self.build.distro_arch_series
@@ -339,8 +347,21 @@ class TestSnapBuild(TestCaseWithFactory):
             self.build.updateStatus(BuildStatus.FAILEDTOBUILD)
         self.assertContentEqual([], self.build.store_upload_jobs)
 
+    def test_updateStatus_fullybuilt_not_configured(self):
+        # A completed SnapBuild does not trigger store uploads if the snap
+        # is not properly configured for that.
+        logger = self.useFixture(FakeLogger())
+        with dbuser(config.builddmaster.dbuser):
+            self.build.updateStatus(BuildStatus.FULLYBUILT)
+        self.assertEqual(0, len(list(self.build.store_upload_jobs)))
+        self.assertIn(
+            "<Snap ~%s/+snap/%s> is not configured for upload to the "
+            "store." % (self.build.snap.owner.name, self.build.snap.name),
+            logger.output.splitlines())
+
     def test_updateStatus_fullybuilt_triggers_store_uploads(self):
         # A completed SnapBuild triggers store uploads.
+        logger = self.useFixture(FakeLogger())
         self.build.snap.store_series = self.factory.makeSnappySeries()
         self.build.snap.store_name = self.factory.getUniqueUnicode()
         self.build.snap.store_upload = True
@@ -348,6 +369,12 @@ class TestSnapBuild(TestCaseWithFactory):
         with dbuser(config.builddmaster.dbuser):
             self.build.updateStatus(BuildStatus.FULLYBUILT)
         self.assertEqual(1, len(list(self.build.store_upload_jobs)))
+        self.assertIn(
+            "Scheduling upload of <SnapBuild ~%s/+snap/%s/+build/%d> to the "
+            "store." % (
+                self.build.snap.owner.name, self.build.snap.name,
+                self.build.id),
+            logger.output.splitlines())
 
     def test_notify_fullybuilt(self):
         # notify does not send mail when a SnapBuild completes normally.