launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/snap-build-channels-ui into lp:launchpad.

Commit message:
Add UI for Snap.auto_build_channels.

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-ui/+merge/344859

In the process of doing this work, it became clear that we're going to need some kind of escape syntax for continuing to install snapcraft using apt in order to make the migration work.  There's more rationale for my choice there in the relevant commit message.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/snap-build-channels-ui into lp:launchpad.

=== 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-04-30 16:52:16 +0000
@@ -80,6 +80,9 @@
 from lp.services.webapp.url import urlappend
 from lp.services.webhooks.browser import WebhookTargetNavigationMixin
 from lp.snappy.browser.widgets.snaparchive import SnapArchiveWidget
+from lp.snappy.browser.widgets.snapbuildchannels import (
+    SnapBuildChannelsWidget,
+    )
 from lp.snappy.browser.widgets.storechannels import StoreChannelsWidget
 from lp.snappy.interfaces.snap import (
     CannotAuthorizeStoreUploads,
@@ -190,6 +193,12 @@
             return 'Built on request'
 
     @property
+    def sorted_auto_build_channels_items(self):
+        if self.context.auto_build_channels is None:
+            return []
+        return sorted(self.context.auto_build_channels.items())
+
+    @property
     def store_channels(self):
         return ', '.join(self.context.store_channels)
 
@@ -322,8 +331,16 @@
         'allow_internet',
         'build_source_tarball',
         'auto_build',
+        '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')
@@ -379,14 +396,16 @@
         'auto_build',
         'auto_build_archive',
         'auto_build_pocket',
+        'auto_build_channels',
         'store_upload',
         'store_name',
         'store_channels',
         ]
     custom_widget('store_distro_series', LaunchpadRadioWidget)
     custom_widget('auto_build_archive', SnapArchiveWidget)
+    custom_widget('auto_build_pocket', LaunchpadDropdownWidget)
+    custom_widget('auto_build_channels', SnapBuildChannelsWidget)
     custom_widget('store_channels', StoreChannelsWidget)
-    custom_widget('auto_build_pocket', LaunchpadDropdownWidget)
 
     help_links = {
         "auto_build_pocket": u"/+help-snappy/snap-build-pocket.html",
@@ -512,6 +531,7 @@
             auto_build=data['auto_build'],
             auto_build_archive=data['auto_build_archive'],
             auto_build_pocket=data['auto_build_pocket'],
+            auto_build_channels=data.get('auto_build_channels'),
             processors=data['processors'], private=private,
             build_source_tarball=data['build_source_tarball'],
             store_upload=data['store_upload'],
@@ -624,6 +644,8 @@
                 del data['auto_build_archive']
             if 'auto_build_pocket' in data:
                 del data['auto_build_pocket']
+            if 'auto_build_channels' in data:
+                del data['auto_build_channels']
         store_upload = data.get('store_upload', False)
         if not store_upload:
             if 'store_name' in data:
@@ -688,16 +710,18 @@
         'auto_build',
         'auto_build_archive',
         'auto_build_pocket',
+        'auto_build_channels',
         'store_upload',
         'store_name',
         'store_channels',
         ]
     custom_widget('store_distro_series', LaunchpadRadioWidget)
-    custom_widget('store_channels', StoreChannelsWidget)
     custom_widget('vcs', LaunchpadRadioWidget)
     custom_widget('git_ref', GitRefWidget, allow_external=True)
     custom_widget('auto_build_archive', SnapArchiveWidget)
     custom_widget('auto_build_pocket', LaunchpadDropdownWidget)
+    custom_widget('auto_build_channels', SnapBuildChannelsWidget)
+    custom_widget('store_channels', StoreChannelsWidget)
 
     help_links = {
         "auto_build_pocket": u"/+help-snappy/snap-build-pocket.html",

=== modified file 'lib/lp/snappy/browser/tests/test_snap.py'
--- lib/lp/snappy/browser/tests/test_snap.py	2018-04-21 10:15:26 +0000
+++ lib/lp/snappy/browser/tests/test_snap.py	2018-04-30 16:52:16 +0000
@@ -254,6 +254,7 @@
         self.assertThat(
             "Pocket for automatic builds:\n\nEdit snap package",
             MatchesTagText(content, "auto_build_pocket"))
+        self.assertIsNone(find_tag_by_id(content, "auto_build_channels"))
         self.assertThat(
             "Builds of this snap package are not automatically uploaded to "
             "the store.\nEdit snap package",
@@ -291,6 +292,7 @@
         self.assertThat(
             "Pocket for automatic builds:\n\nEdit snap package",
             MatchesTagText(content, "auto_build_pocket"))
+        self.assertIsNone(find_tag_by_id(content, "auto_build_channels"))
         self.assertThat(
             "Builds of this snap package are not automatically uploaded to "
             "the store.\nEdit snap package",
@@ -389,6 +391,10 @@
         browser.getControl(name="field.auto_build_archive.ppa").value = (
             archive.reference)
         browser.getControl("Pocket for automatic builds").value = ["SECURITY"]
+        browser.getControl(
+            name="field.auto_build_channels.core").value = "stable"
+        browser.getControl(
+            name="field.auto_build_channels.snapcraft").value = "edge"
         browser.getControl("Create snap package").click()
 
         content = find_main_content(browser.contents)
@@ -402,6 +408,10 @@
         self.assertThat(
             "Pocket for automatic builds:\nSecurity\nEdit snap package",
             MatchesTagText(content, "auto_build_pocket"))
+        self.assertThat(
+            "Source snap channels for automatic builds:\nEdit snap package\n"
+            "core\nstable\nsnapcraft\nedge\n",
+            MatchesTagText(content, "auto_build_channels"))
 
     def test_create_new_snap_store_upload(self):
         # Creating a new snap and asking for it to be automatically uploaded
@@ -713,6 +723,8 @@
         browser.getControl(name="field.auto_build_archive.ppa").value = (
             archive.reference)
         browser.getControl("Pocket for automatic builds").value = ["SECURITY"]
+        browser.getControl(
+            name="field.auto_build_channels.snapcraft").value = "edge"
         browser.getControl("Update snap package").click()
 
         content = find_main_content(browser.contents)
@@ -739,6 +751,10 @@
             "Pocket for automatic builds:\nSecurity\nEdit snap package",
             MatchesTagText(content, "auto_build_pocket"))
         self.assertThat(
+            "Source snap channels for automatic builds:\nEdit snap package\n"
+            "snapcraft\nedge",
+            MatchesTagText(content, "auto_build_channels"))
+        self.assertThat(
             "Builds of this snap package are not automatically uploaded to "
             "the store.\nEdit snap package",
             MatchesTagText(content, "store_upload"))

=== added file 'lib/lp/snappy/browser/widgets/snapbuildchannels.py'
--- lib/lp/snappy/browser/widgets/snapbuildchannels.py	1970-01-01 00:00:00 +0000
+++ lib/lp/snappy/browser/widgets/snapbuildchannels.py	2018-04-30 16:52:16 +0000
@@ -0,0 +1,99 @@
+# Copyright 2018 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""XXX: Module docstring goes here."""
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    'SnapBuildChannelsWidget',
+    ]
+
+from z3c.ptcompat import ViewPageTemplateFile
+from zope.formlib.interfaces import IInputWidget
+from zope.formlib.utility import setUpWidget
+from zope.formlib.widget import (
+    BrowserWidget,
+    InputErrors,
+    InputWidget,
+    )
+from zope.interface import implementer
+from zope.schema import TextLine
+from zope.security.proxy import isinstance as zope_isinstance
+
+from lp.app.errors import UnexpectedFormData
+from lp.services.webapp.interfaces import (
+    IAlwaysSubmittedWidget,
+    ISingleLineWidgetLayout,
+    )
+
+
+@implementer(ISingleLineWidgetLayout, IAlwaysSubmittedWidget, IInputWidget)
+class SnapBuildChannelsWidget(BrowserWidget, InputWidget):
+
+    template = ViewPageTemplateFile("templates/snapbuildchannels.pt")
+    hint = False
+    snap_names = ["core", "snapcraft"]
+    _widgets_set_up = False
+
+    def setUpSubWidgets(self):
+        if self._widgets_set_up:
+            return
+        fields = [
+            TextLine(
+                __name__=snap_name, title="%s channel" % snap_name,
+                required=False)
+            for snap_name in self.snap_names
+            ]
+        for field in fields:
+            setUpWidget(
+                self, field.__name__, field, IInputWidget, prefix=self.name)
+        self._widgets_set_up = True
+
+    def setRenderedValue(self, value):
+        """See `IWidget`."""
+        self.setUpSubWidgets()
+        if not zope_isinstance(value, dict):
+            value = {}
+        self.core_widget.setRenderedValue(value.get("core"))
+        self.snapcraft_widget.setRenderedValue(value.get("snapcraft"))
+
+    def hasInput(self):
+        """See `IInputWidget`."""
+        return any(
+            "%s.%s" % (self.name, snap_name) in self.request.form
+            for snap_name in self.snap_names)
+
+    def hasValidInput(self):
+        """See `IInputWidget`."""
+        try:
+            self.getInputValue()
+            return True
+        except (InputErrors, UnexpectedFormData):
+            return False
+
+    def getInputValue(self):
+        """See `IInputWidget`."""
+        self.setUpSubWidgets()
+        channels = {}
+        for snap_name in self.snap_names:
+            widget = getattr(self, snap_name + "_widget")
+            channel = widget.getInputValue()
+            if channel:
+                channels[snap_name] = channel
+        return channels
+
+    def error(self):
+        """See `IBrowserWidget`."""
+        try:
+            if self.hasInput():
+                self.getInputValue()
+        except InputErrors as error:
+            self._error = error
+        return super(SnapBuildChannelsWidget, self).error()
+
+    def __call__(self):
+        """See `IBrowserWidget`."""
+        self.setUpSubWidgets()
+        return self.template()

=== added file 'lib/lp/snappy/browser/widgets/templates/snapbuildchannels.pt'
--- lib/lp/snappy/browser/widgets/templates/snapbuildchannels.pt	1970-01-01 00:00:00 +0000
+++ lib/lp/snappy/browser/widgets/templates/snapbuildchannels.pt	2018-04-30 16:52:16 +0000
@@ -0,0 +1,16 @@
+<tal:root
+    xmlns:tal="http://xml.zope.org/namespaces/tal";
+    omit-tag="">
+
+<table class="subordinate">
+  <tr>
+    <td>core</td>
+    <td><div tal:content="structure view/core_widget" /></td>
+  </tr>
+  <tr>
+    <td>snapcraft</td>
+    <td><div tal:content="structure view/snapcraft_widget" /></td>
+  </tr>
+</table>
+
+</tal:root>

=== added file 'lib/lp/snappy/browser/widgets/tests/test_snapbuildchannelswidget.py'
--- lib/lp/snappy/browser/widgets/tests/test_snapbuildchannelswidget.py	1970-01-01 00:00:00 +0000
+++ lib/lp/snappy/browser/widgets/tests/test_snapbuildchannelswidget.py	2018-04-30 16:52:16 +0000
@@ -0,0 +1,132 @@
+# Copyright 2018 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+from __future__ import absolute_import, print_function, unicode_literals
+
+__metaclass__ = type
+
+import re
+
+from zope.formlib.interfaces import (
+    IBrowserWidget,
+    IInputWidget,
+    )
+from zope.schema import Dict
+
+from lp.services.beautifulsoup import BeautifulSoup
+from lp.services.webapp.servers import LaunchpadTestRequest
+from lp.snappy.browser.widgets.snapbuildchannels import (
+    SnapBuildChannelsWidget,
+    )
+from lp.testing import (
+    TestCaseWithFactory,
+    verifyObject,
+    )
+from lp.testing.layers import DatabaseFunctionalLayer
+
+
+class TestSnapBuildChannelsWidget(TestCaseWithFactory):
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestSnapBuildChannelsWidget, self).setUp()
+        field = Dict(
+            __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)
+
+    def test_implements(self):
+        self.assertTrue(verifyObject(IBrowserWidget, self.widget))
+        self.assertTrue(verifyObject(IInputWidget, self.widget))
+
+    def test_template(self):
+        self.assertTrue(
+            self.widget.template.filename.endswith("snapbuildchannels.pt"),
+            "Template was not set up.")
+
+    def test_setUpSubWidgets_first_call(self):
+        # The subwidgets are set up and a flag is set.
+        self.widget.setUpSubWidgets()
+        self.assertTrue(self.widget._widgets_set_up)
+        self.assertIsNotNone(getattr(self.widget, "core_widget", None))
+        self.assertIsNotNone(getattr(self.widget, "snapcraft_widget", None))
+
+    def test_setUpSubWidgets_second_call(self):
+        # The setUpSubWidgets method exits early if a flag is set to
+        # indicate that the widgets were set up.
+        self.widget._widgets_set_up = True
+        self.widget.setUpSubWidgets()
+        self.assertIsNone(getattr(self.widget, "core_widget", None))
+        self.assertIsNone(getattr(self.widget, "snapcraft_widget", None))
+
+    def test_setRenderedValue_None(self):
+        self.widget.setRenderedValue(None)
+        self.assertIsNone(self.widget.core_widget._getCurrentValue())
+        self.assertIsNone(self.widget.snapcraft_widget._getCurrentValue())
+
+    def test_setRenderedValue_empty(self):
+        self.widget.setRenderedValue({})
+        self.assertIsNone(self.widget.core_widget._getCurrentValue())
+        self.assertIsNone(self.widget.snapcraft_widget._getCurrentValue())
+
+    def test_setRenderedValue_one_channel(self):
+        self.widget.setRenderedValue({"snapcraft": "stable"})
+        self.assertIsNone(self.widget.core_widget._getCurrentValue())
+        self.assertEqual(
+            "stable", self.widget.snapcraft_widget._getCurrentValue())
+
+    def test_setRenderedValue_all_channels(self):
+        self.widget.setRenderedValue(
+            {"core": "candidate", "snapcraft": "stable"})
+        self.assertEqual(
+            "candidate", self.widget.core_widget._getCurrentValue())
+        self.assertEqual(
+            "stable", self.widget.snapcraft_widget._getCurrentValue())
+
+    def test_hasInput_false(self):
+        # hasInput is false when there are no channels in the form data.
+        self.widget.request = LaunchpadTestRequest(form={})
+        self.assertFalse(self.widget.hasInput())
+
+    def test_hasInput_true(self):
+        # hasInput is true when there are channels in the form data.
+        self.widget.request = LaunchpadTestRequest(
+            form={"field.auto_build_channels.snapcraft": "stable"})
+        self.assertTrue(self.widget.hasInput())
+
+    def test_hasValidInput_true(self):
+        # The field input is valid when all submitted channels are valid.
+        # (At the moment, individual channel names are not validated, so
+        # there is no "false" counterpart to this test.)
+        form = {
+            "field.auto_build_channels.core": "",
+            "field.auto_build_channels.snapcraft": "stable",
+            }
+        self.widget.request = LaunchpadTestRequest(form=form)
+        self.assertTrue(self.widget.hasValidInput())
+
+    def test_getInputValue(self):
+        form = {
+            "field.auto_build_channels.core": "",
+            "field.auto_build_channels.snapcraft": "stable",
+            }
+        self.widget.request = LaunchpadTestRequest(form=form)
+        self.assertEqual({"snapcraft": "stable"}, self.widget.getInputValue())
+
+    def test_call(self):
+        # The __call__ method sets up the widgets.
+        markup = self.widget()
+        self.assertIsNotNone(self.widget.core_widget)
+        self.assertIsNotNone(self.widget.snapcraft_widget)
+        soup = BeautifulSoup(markup)
+        fields = soup.findAll(["input"], {"id": re.compile(".*")})
+        expected_ids = [
+            "field.auto_build_channels.core",
+            "field.auto_build_channels.snapcraft",
+            ]
+        ids = [field["id"] for field in fields]
+        self.assertContentEqual(expected_ids, ids)

=== modified file 'lib/lp/snappy/model/snapbuildbehaviour.py'
--- lib/lp/snappy/model/snapbuildbehaviour.py	2018-04-23 11:14:38 +0000
+++ lib/lp/snappy/model/snapbuildbehaviour.py	2018-04-30 16:52:16 +0000
@@ -110,7 +110,8 @@
                 logger=logger))
         args["archive_private"] = build.archive.private
         args["build_url"] = canonical_url(build)
-        if build.channels is not None:
+        if (build.channels is not None and
+                build.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.

=== modified file 'lib/lp/snappy/templates/snap-edit.pt'
--- lib/lp/snappy/templates/snap-edit.pt	2018-04-21 10:01:22 +0000
+++ lib/lp/snappy/templates/snap-edit.pt	2018-04-30 16:52:16 +0000
@@ -76,6 +76,9 @@
               <tal:widget define="widget nocall:view/widgets/auto_build_pocket">
                 <metal:block use-macro="context/@@launchpad_form/widget_row" />
               </tal:widget>
+              <tal:widget define="widget nocall:view/widgets/auto_build_channels">
+                <metal:block use-macro="context/@@launchpad_form/widget_row" />
+              </tal:widget>
             </table>
           </td>
         </tr>

=== modified file 'lib/lp/snappy/templates/snap-index.pt'
--- lib/lp/snappy/templates/snap-index.pt	2018-04-21 10:15:26 +0000
+++ lib/lp/snappy/templates/snap-index.pt	2018-04-30 16:52:16 +0000
@@ -95,6 +95,21 @@
           <a tal:replace="structure view/menu:overview/edit/fmt:icon"/>
         </dd>
       </dl>
+      <dl id="auto_build_channels" tal:condition="context/auto_build_channels">
+        <dt>
+          Source snap channels for automatic builds:
+          <a tal:replace="structure view/menu:overview/edit/fmt:icon"/>
+        </dt>
+        <dd>
+          <table class="listing compressed">
+            <tbody>
+              <tr tal:repeat="pair view/sorted_auto_build_channels_items">
+                <td tal:repeat="value pair" tal:content="value"/>
+              </tr>
+            </tbody>
+          </table>
+        </dd>
+      </dl>
     </div>
 
     <div id="store_upload" class="two-column-list"

=== modified file 'lib/lp/snappy/templates/snap-new.pt'
--- lib/lp/snappy/templates/snap-new.pt	2018-04-21 10:01:22 +0000
+++ lib/lp/snappy/templates/snap-new.pt	2018-04-30 16:52:16 +0000
@@ -51,6 +51,9 @@
               <tal:widget define="widget nocall:view/widgets/auto_build_pocket">
                 <metal:block use-macro="context/@@launchpad_form/widget_row" />
               </tal:widget>
+              <tal:widget define="widget nocall:view/widgets/auto_build_channels">
+                <metal:block use-macro="context/@@launchpad_form/widget_row" />
+              </tal:widget>
             </table>
           </td>
         </tr>

=== modified file 'lib/lp/snappy/tests/test_snapbuildbehaviour.py'
--- lib/lp/snappy/tests/test_snapbuildbehaviour.py	2018-04-23 11:14:38 +0000
+++ lib/lp/snappy/tests/test_snapbuildbehaviour.py	2018-04-30 16:52:16 +0000
@@ -428,6 +428,16 @@
         self.assertEqual({"snapcraft": "edge"}, args["channels"])
 
     @defer.inlineCallbacks
+    def test_extraBuildArgs_channels_apt(self):
+        # {"snapcraft": "apt"} causes snapcraft to be installed from apt.
+        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.