launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-build-channels-feature-flag into lp:launchpad with lp:~cjwatson/launchpad/snap-build-channels-ui as a prerequisite.

Commit message:
Add a feature flag to control the installation source for snapcraft.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1737994 in Launchpad itself: "build.snapcraft.io uses snapcraft from the deb archive"
  https://bugs.launchpad.net/launchpad/+bug/1737994

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/snap-build-channels-feature-flag/+merge/344868
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-build-channels-feature-flag into lp:launchpad.

=== modified file 'lib/lp/snappy/browser/snap.py'
--- lib/lp/snappy/browser/snap.py	2018-04-30 22:27:39 +0000
+++ lib/lp/snappy/browser/snap.py	2018-04-30 22:27:40 +0000
@@ -334,13 +334,6 @@
         'auto_build_channels',
         'store_upload',
         ])
-    auto_build_channels = copy_field(
-        ISnap['auto_build_channels'],
-        description=(
-            u'The channels to use for build tools when building the snap '
-            u'package.  If unset, or if the channel for snapcraft is set to '
-            u'"apt", the default behaviour is to install snapcraft from the '
-            u'source archive using apt.'))
     store_distro_series = Choice(
         vocabulary='BuildableSnappyDistroSeries', required=True,
         title=u'Series')

=== modified file 'lib/lp/snappy/browser/widgets/snapbuildchannels.py'
--- lib/lp/snappy/browser/widgets/snapbuildchannels.py	2018-04-30 22:27:39 +0000
+++ lib/lp/snappy/browser/widgets/snapbuildchannels.py	2018-04-30 22:27:40 +0000
@@ -23,10 +23,12 @@
 from zope.security.proxy import isinstance as zope_isinstance
 
 from lp.app.errors import UnexpectedFormData
+from lp.services.features import getFeatureFlag
 from lp.services.webapp.interfaces import (
     IAlwaysSubmittedWidget,
     ISingleLineWidgetLayout,
     )
+from lp.snappy.interfaces.snap import SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG
 
 
 @implementer(ISingleLineWidgetLayout, IAlwaysSubmittedWidget, IInputWidget)
@@ -37,6 +39,25 @@
     snap_names = ["core", "snapcraft"]
     _widgets_set_up = False
 
+    def __init__(self, context, request):
+        super(SnapBuildChannelsWidget, self).__init__(context, request)
+        self.hint = (
+            'The channels to use for build tools when building the snap '
+            'package.\n')
+        default_snapcraft_channel = (
+            getFeatureFlag(SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG) or "apt")
+        if default_snapcraft_channel == "apt":
+            self.hint += (
+                'If unset, or if the channel for snapcraft is set to "apt", '
+                'the default is to install snapcraft from the source archive '
+                'using apt.')
+        else:
+            self.hint += (
+                'If unset, the default is to install snapcraft from the "%s" '
+                'channel.  Setting the channel for snapcraft to "apt" causes '
+                'snapcraft to be installed from the source archive using '
+                'apt.' % default_snapcraft_channel)
+
     def setUpSubWidgets(self):
         if self._widgets_set_up:
             return

=== modified file 'lib/lp/snappy/browser/widgets/tests/test_snapbuildchannelswidget.py'
--- lib/lp/snappy/browser/widgets/tests/test_snapbuildchannelswidget.py	2018-04-30 22:27:39 +0000
+++ lib/lp/snappy/browser/widgets/tests/test_snapbuildchannelswidget.py	2018-04-30 22:27:40 +0000
@@ -14,10 +14,12 @@
 from zope.schema import Dict
 
 from lp.services.beautifulsoup import BeautifulSoup
+from lp.services.features.testing import FeatureFixture
 from lp.services.webapp.servers import LaunchpadTestRequest
 from lp.snappy.browser.widgets.snapbuildchannels import (
     SnapBuildChannelsWidget,
     )
+from lp.snappy.interfaces.snap import SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG
 from lp.testing import (
     TestCaseWithFactory,
     verifyObject,
@@ -35,9 +37,9 @@
             __name__="auto_build_channels",
             title="Source snap channels for automatic builds")
         self.context = self.factory.makeSnap()
-        field = field.bind(self.context)
-        request = LaunchpadTestRequest()
-        self.widget = SnapBuildChannelsWidget(field, request)
+        self.field = field.bind(self.context)
+        self.request = LaunchpadTestRequest()
+        self.widget = SnapBuildChannelsWidget(self.field, self.request)
 
     def test_implements(self):
         self.assertTrue(verifyObject(IBrowserWidget, self.widget))
@@ -48,6 +50,40 @@
             self.widget.template.filename.endswith("snapbuildchannels.pt"),
             "Template was not set up.")
 
+    def test_hint_no_feature_flag(self):
+        self.assertEqual(
+            'The channels to use for build tools when building the snap '
+            'package.\n'
+            'If unset, or if the channel for snapcraft is set to "apt", '
+            'the default is to install snapcraft from the source archive '
+            'using apt.',
+            self.widget.hint)
+
+    def test_hint_feature_flag_apt(self):
+        self.useFixture(
+            FeatureFixture({SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG: "apt"}))
+        widget = SnapBuildChannelsWidget(self.field, self.request)
+        self.assertEqual(
+            'The channels to use for build tools when building the snap '
+            'package.\n'
+            'If unset, or if the channel for snapcraft is set to "apt", '
+            'the default is to install snapcraft from the source archive '
+            'using apt.',
+            widget.hint)
+
+    def test_hint_feature_flag_real_channel(self):
+        self.useFixture(
+            FeatureFixture({SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG: "stable"}))
+        widget = SnapBuildChannelsWidget(self.field, self.request)
+        self.assertEqual(
+            'The channels to use for build tools when building the snap '
+            'package.\n'
+            'If unset, the default is to install snapcraft from the "stable" '
+            'channel.  Setting the channel for snapcraft to "apt" causes '
+            'snapcraft to be installed from the source archive using '
+            'apt.',
+            widget.hint)
+
     def test_setUpSubWidgets_first_call(self):
         # The subwidgets are set up and a flag is set.
         self.widget.setUpSubWidgets()

=== modified file 'lib/lp/snappy/interfaces/snap.py'
--- lib/lp/snappy/interfaces/snap.py	2018-04-21 10:01:22 +0000
+++ lib/lp/snappy/interfaces/snap.py	2018-04-30 22:27:40 +0000
@@ -19,6 +19,7 @@
     'NoSourceForSnap',
     'NoSuchSnap',
     'SNAP_PRIVATE_FEATURE_FLAG',
+    'SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG',
     'SNAP_TESTING_FLAGS',
     'SNAP_WEBHOOKS_FEATURE_FLAG',
     'SnapAuthorizationBadMacaroon',
@@ -102,6 +103,7 @@
 
 
 SNAP_PRIVATE_FEATURE_FLAG = u"snap.allow_private"
+SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG = u"snap.channels.snapcraft"
 SNAP_WEBHOOKS_FEATURE_FLAG = u"snap.webhooks.enabled"
 
 

=== modified file 'lib/lp/snappy/model/snapbuildbehaviour.py'
--- lib/lp/snappy/model/snapbuildbehaviour.py	2018-04-30 22:27:39 +0000
+++ lib/lp/snappy/model/snapbuildbehaviour.py	2018-04-30 22:27:40 +0000
@@ -30,8 +30,12 @@
     )
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.config import config
+from lp.services.features import getFeatureFlag
 from lp.services.webapp import canonical_url
-from lp.snappy.interfaces.snap import SnapBuildArchiveOwnerMismatch
+from lp.snappy.interfaces.snap import (
+    SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG,
+    SnapBuildArchiveOwnerMismatch,
+    )
 from lp.snappy.interfaces.snapbuild import ISnapBuild
 from lp.soyuz.adapters.archivedependencies import (
     get_sources_list_for_building,
@@ -110,12 +114,15 @@
                 logger=logger))
         args["archive_private"] = build.archive.private
         args["build_url"] = canonical_url(build)
-        if (build.channels is not None and
-                build.channels.get("snapcraft") != "apt"):
+        channels = build.channels or {}
+        if "snapcraft" not in channels:
+            channels["snapcraft"] = (
+                getFeatureFlag(SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG) or "apt")
+        if channels.get("snapcraft") != "apt":
             # We have to remove the security proxy that Zope applies to this
             # dict, since otherwise we'll be unable to serialise it to
             # XML-RPC.
-            args["channels"] = removeSecurityProxy(build.channels)
+            args["channels"] = removeSecurityProxy(channels)
         if build.snap.branch is not None:
             args["branch"] = build.snap.branch.bzr_identity
         elif build.snap.git_ref is not None:

=== modified file 'lib/lp/snappy/tests/test_snapbuildbehaviour.py'
--- lib/lp/snappy/tests/test_snapbuildbehaviour.py	2018-04-30 22:27:39 +0000
+++ lib/lp/snappy/tests/test_snapbuildbehaviour.py	2018-04-30 22:27:40 +0000
@@ -55,9 +55,13 @@
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.config import config
+from lp.services.features.testing import FeatureFixture
 from lp.services.log.logger import BufferLogger
 from lp.services.webapp import canonical_url
-from lp.snappy.interfaces.snap import SnapBuildArchiveOwnerMismatch
+from lp.snappy.interfaces.snap import (
+    SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG,
+    SnapBuildArchiveOwnerMismatch,
+    )
 from lp.snappy.model.snapbuildbehaviour import SnapBuildBehaviour
 from lp.soyuz.adapters.archivedependencies import (
     get_sources_list_for_building,
@@ -438,6 +442,33 @@
         self.assertNotIn("channels", args)
 
     @defer.inlineCallbacks
+    def test_extraBuildArgs_channels_feature_flag_real_channel(self):
+        # If the snap.channels.snapcraft feature flag is set, it identifies
+        # the default channel to be used for snapcraft.
+        self.useFixture(
+            FeatureFixture({SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG: "stable"}))
+        job = self.makeJob()
+        expected_archives, expected_trusted_keys = (
+            yield get_sources_list_for_building(
+                job.build, job.build.distro_arch_series, None))
+        args = yield job._extraBuildArgs()
+        self.assertFalse(isProxy(args["channels"]))
+        self.assertEqual({"snapcraft": "stable"}, args["channels"])
+
+    @defer.inlineCallbacks
+    def test_extraBuildArgs_channels_feature_flag_overridden(self):
+        # The snap.channels.snapcraft feature flag can be overridden by
+        # explicit configuration.
+        self.useFixture(
+            FeatureFixture({SNAP_SNAPCRAFT_CHANNEL_FEATURE_FLAG: "stable"}))
+        job = self.makeJob(channels={"snapcraft": "apt"})
+        expected_archives, expected_trusted_keys = (
+            yield get_sources_list_for_building(
+                job.build, job.build.distro_arch_series, None))
+        args = yield job._extraBuildArgs()
+        self.assertNotIn("channels", args)
+
+    @defer.inlineCallbacks
     def test_extraBuildArgs_disallow_internet(self):
         # If external network access is not allowed for the snap,
         # _extraBuildArgs does not dispatch a proxy token.