Merge ~morphis/snappy-hwe-snaps/+git/wifi-ap:feature/config-disable-by-default into ~snappy-hwe-team/snappy-hwe-snaps/+git/wifi-ap:master

Proposed by Simon Fels
Status: Merged
Approved by: Jim Hodapp
Approved revision: 66545e285ebc3ce33ea312e938f994a5c44a8ef0
Merged at revision: 724d7feea229d555fe37ee3436534a0c63a1e0f5
Proposed branch: ~morphis/snappy-hwe-snaps/+git/wifi-ap:feature/config-disable-by-default
Merge into: ~snappy-hwe-team/snappy-hwe-snaps/+git/wifi-ap:master
Diff against target: 316 lines (+185/-19)
10 files modified
bin/automatic-setup.sh (+2/-0)
hooks/configure (+40/-0)
snapcraft.yaml (+6/-0)
spread.yaml (+14/-0)
tests/image/create-image.sh (+3/-0)
tests/lib/prepare.sh (+3/-18)
tests/lib/utilities.sh (+21/-0)
tests/main/conf-wizard-auto-can-be-blocked/task.yaml (+32/-0)
tests/main/conf-wizard-disabled-from-gadget/task.yaml (+46/-0)
tests/main/utf8-ssid/task.yaml (+18/-1)
Reviewer Review Type Date Requested Status
Jim Hodapp (community) Approve
System Enablement Bot continuous-integration Approve
Matteo Croce (community) Approve
Review via email: mp+316120@code.launchpad.net

This proposal supersedes a proposal from 2017-02-01.

Description of the change

Add configure option to allow disabling automatic setup from the gadget snap.

The test case needs to stay manual until a first version of the snap
with a configure hook is released into the edge channel.

To post a comment you must log in.
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Revision history for this message
Simon Fels (morphis) wrote : Posted in a previous version of this proposal

Please hold-off reviewing this as the state of this MP doesn't reflect what is pushed in the git repository. There seems to be a quite huge delay within the launchpad infrastructure today.

Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Jim Hodapp (jhodapp) wrote :

Looks good! Several minor changes inline below.

review: Needs Fixing
Revision history for this message
Simon Fels (morphis) :
Revision history for this message
Simon Fels (morphis) wrote :

Fixed all review comments

Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Matteo Croce (teknoraver) :
review: Approve
Revision history for this message
Jim Hodapp (jhodapp) wrote :

One last question I need answered before I'm happy with this MR.

review: Needs Information
Revision history for this message
Simon Fels (morphis) :
Revision history for this message
System Enablement Bot (system-enablement-ci-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Jim Hodapp (jhodapp) :
Revision history for this message
Jim Hodapp (jhodapp) wrote :

LGTM

review: Approve
Revision history for this message
Jim Hodapp (jhodapp) wrote :

Note: I'm top approving after speaking with Simon about this MR since the only other maintainer (Matteo) is not around today.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/bin/automatic-setup.sh b/bin/automatic-setup.sh
2index 9a93954..5c86bb2 100755
3--- a/bin/automatic-setup.sh
4+++ b/bin/automatic-setup.sh
5@@ -16,6 +16,8 @@
6
7 set -x
8
9+[ -f "$SNAP_COMMON/.block_auto_wizard" ] && exit 0
10+
11 [ -f "$SNAP_DATA/config" ] && exit 0
12
13 while ! $SNAP/bin/client status; do
14diff --git a/hooks/configure b/hooks/configure
15new file mode 100755
16index 0000000..dd0482a
17--- /dev/null
18+++ b/hooks/configure
19@@ -0,0 +1,40 @@
20+#!/bin/sh
21+#
22+# Copyright (C) 2017 Canonical Ltd
23+#
24+# This program is free software: you can redistribute it and/or modify
25+# it under the terms of the GNU General Public License version 3 as
26+# published by the Free Software Foundation.
27+#
28+# This program is distributed in the hope that it will be useful,
29+# but WITHOUT ANY WARRANTY; without even the implied warranty of
30+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
31+# GNU General Public License for more details.
32+#
33+# You should have received a copy of the GNU General Public License
34+# along with this program. If not, see <http://www.gnu.org/licenses/>.
35+
36+set -x
37+
38+# The 'automatic-setup.disabled' option is only meant to be used from
39+# a gadget snap to prevent the wizard from running when the snap is
40+# being installed. As the configure hook will be called before the
41+# services are being started we can easily create an empty
42+# configuration to fall back to the default one for everything.
43+value=`snapctl get automatic-setup.disable`
44+if [ -n "$value" ]; then
45+ case "$value" in
46+ true)
47+ touch $SNAP_COMMON/.block_auto_wizard
48+ ;;
49+ false)
50+ rm $SNAP_COMMON/.block_auto_wizard || true
51+ ;;
52+ *)
53+ echo "ERROR: invalid value '$value' provided for automatic-setup.disable"
54+ exit 1
55+ esac
56+else
57+ # If no value provided set up our default
58+ snapctl set automatic-setup.disable=false
59+fi
60diff --git a/snapcraft.yaml b/snapcraft.yaml
61index 6123d4b..a3e20db 100644
62--- a/snapcraft.yaml
63+++ b/snapcraft.yaml
64@@ -45,6 +45,12 @@ apps:
65 - network
66
67 parts:
68+ hooks:
69+ plugin: dump
70+ source: hooks
71+ organize:
72+ configure: meta/hooks/configure
73+
74 common:
75 plugin: dump
76 source: .
77diff --git a/spread.yaml b/spread.yaml
78index 74a0ff8..4bea33a 100644
79--- a/spread.yaml
80+++ b/spread.yaml
81@@ -39,6 +39,20 @@ exclude:
82 prepare: |
83 . $TESTSLIB/prepare-all.sh
84
85+prepare-each: |
86+ # Cleanup logs so we can just dump what has happened in the debug-each
87+ # step below after a test case ran.
88+ journalctl --rotate
89+ sleep .1
90+ journalctl --vacuum-time=1ms
91+ dmesg -c > /dev/null
92+
93+debug-each: |
94+ journalctl
95+ dmesg | grep DENIED || true
96+
97+kill-timeout: 1h
98+
99 suites:
100 tests/main/:
101 summary: Full-system tests for the wifi-ap snap
102diff --git a/tests/image/create-image.sh b/tests/image/create-image.sh
103index 33c1a37..5e68bc7 100755
104--- a/tests/image/create-image.sh
105+++ b/tests/image/create-image.sh
106@@ -40,8 +40,11 @@ if [ ! -z "$snap" ] ; then
107 ubuntu_image_extra_args="--extra-snaps $snap"
108 fi
109
110+snap download --stable pc
111+
112 ubuntu-image \
113 --channel $channel \
114+ --extra-snaps pc_*.snap \
115 -o $image_name \
116 --image-size 4G \
117 $ubuntu_image_extra_args \
118diff --git a/tests/lib/prepare.sh b/tests/lib/prepare.sh
119index b95e884..d4b7020 100644
120--- a/tests/lib/prepare.sh
121+++ b/tests/lib/prepare.sh
122@@ -1,5 +1,7 @@
123 #!/bin/bash
124
125+. $TESTSLIB/utilities.sh
126+
127 echo "Wait for firstboot change to be ready"
128 while ! snap changes | grep -q "Done"; do
129 snap changes || true
130@@ -21,24 +23,7 @@ done
131 echo "Kernel has a store revision"
132 snap list | grep ^${kernel_name} | grep -E " [0-9]+\s+canonical"
133
134-# If we don't install wifi-ap here we get a system
135-# without any network connectivity after reboot.
136-if [ -n "$SNAP_CHANNEL" ] ; then
137- # Don't reinstall if we have it installed already
138- if ! snap list | grep wifi-ap ; then
139- snap install --$SNAP_CHANNEL wifi-ap
140- fi
141-else
142- # Install prebuilt wifi-ap snap
143- snap install --dangerous /home/wifi-ap/wifi-ap_*_amd64.snap
144- # As we have a snap which we build locally it's unasserted and therefore
145- # we don't have any snap-declarations in place and need to manually
146- # connect all plugs.
147- snap connect wifi-ap:network-control core
148- snap connect wifi-ap:network-bind core
149- snap connect wifi-ap:network core
150- snap connect wifi-ap:firewall-control core
151-fi
152+install_snap_under_test
153
154 # Snapshot of the current snapd state for a later restore
155 if [ ! -f $SPREAD_PATH/snapd-state.tar.gz ] ; then
156diff --git a/tests/lib/utilities.sh b/tests/lib/utilities.sh
157index 9e2513e..83f2e9e 100644
158--- a/tests/lib/utilities.sh
159+++ b/tests/lib/utilities.sh
160@@ -24,3 +24,24 @@ wait_until_interface_is_available() {
161 sleep 0.2
162 done
163 }
164+
165+install_snap_under_test() {
166+ # If we don't install wifi-ap here we get a system
167+ # without any network connectivity after reboot.
168+ if [ -n "$SNAP_CHANNEL" ] ; then
169+ # Don't reinstall if we have it installed already
170+ if ! snap list | grep wifi-ap ; then
171+ snap install --$SNAP_CHANNEL wifi-ap
172+ fi
173+ else
174+ # Install prebuilt wifi-ap snap
175+ snap install --dangerous /home/wifi-ap/wifi-ap_*_amd64.snap
176+ # As we have a snap which we build locally it's unasserted and therefore
177+ # we don't have any snap-declarations in place and need to manually
178+ # connect all plugs.
179+ snap connect wifi-ap:network-control core
180+ snap connect wifi-ap:network-bind core
181+ snap connect wifi-ap:network core
182+ snap connect wifi-ap:firewall-control core
183+ fi
184+}
185diff --git a/tests/main/conf-wizard-auto-can-be-blocked/task.yaml b/tests/main/conf-wizard-auto-can-be-blocked/task.yaml
186new file mode 100644
187index 0000000..ecacd6f
188--- /dev/null
189+++ b/tests/main/conf-wizard-auto-can-be-blocked/task.yaml
190@@ -0,0 +1,32 @@
191+summary: Verify the automatic wizard running on service startup can be blocked with a configuration option
192+
193+environment:
194+ SNAP_COMMON: /var/snap/wifi-ap/common
195+ SNAP_DATA: /var/snap/wifi-ap/current
196+
197+execute: |
198+ # The service was already started so clean up everything
199+ systemctl stop snap.wifi-ap.automatic-setup
200+
201+ rm -f $SNAP_DATA/config
202+ rm -f $SNAP_COMMON/.block_auto_wizard
203+
204+ test "`snap get wifi-ap automatic-setup.disable`" = "false"
205+
206+ # Setting the configuration option should put the block in place
207+ snap set wifi-ap automatic-setup.disable=true
208+ test -e $SNAP_COMMON/.block_auto_wizard
209+ test "`snap get wifi-ap automatic-setup.disable`" = "true"
210+
211+ # Starting the automatic setup service should not create a configuration
212+ # file for the management service
213+ systemctl start snap.wifi-ap.automatic-setup
214+ test ! -e $SNAP_DATA/config
215+
216+ # Now lets disable the block again and ensure the automatic wizard works
217+ # again as normal
218+ snap set wifi-ap automatic-setup.disable=false
219+ test ! -e $SNAP_COMMON/.block_auto_wizard
220+ systemctl restart snap.wifi-ap.automatic-setup
221+ sleep 1
222+ test -e $SNAP_DATA/config
223diff --git a/tests/main/conf-wizard-disabled-from-gadget/task.yaml b/tests/main/conf-wizard-disabled-from-gadget/task.yaml
224new file mode 100644
225index 0000000..8907db3
226--- /dev/null
227+++ b/tests/main/conf-wizard-disabled-from-gadget/task.yaml
228@@ -0,0 +1,46 @@
229+summary: Verify the wizard can be disabled by default from a gadget snap
230+
231+# FIXME: This test needs to stay manual until a snap is in edge which
232+# contains a configure hook. Otherwise the snap installation will
233+# always fail.
234+manual: true
235+
236+prepare: |
237+ . $TESTSLIB/snap-names.sh
238+ snap list | grep $gadget_name | awk '{print $2}' > /tmp/gadget_version
239+
240+restore: |
241+ . $TESTSLIB/snap-names.sh
242+ # Restore the original gadget snap so that any following tests don't suffer
243+ # from our modified gadget.
244+ original_revision=`cat /tmp/gadget_version`
245+ current_revision=`snap list | grep $gadget_name | awk '{print $2}'`
246+ if [ $current_revision -ne $original_revision ]; then
247+ snap revert --revision=$original_revision $gadget_name
248+ fi
249+
250+execute: |
251+ . $TESTSLIB/snap-names.sh
252+ . $TESTSLIB/utilities.sh
253+
254+ snap remove wifi-ap
255+ snap install --edge --devmode se-test-tools
256+
257+ # We need a custom gadget snap for this so lets fetch one from the store
258+ # and modify it.
259+ snap download --stable $gadget_name
260+ /snap/bin/se-test-tools.unsquashfs -d gadget ${gadget_name}_*.snap
261+ cat << EOF >> gadget/meta/gadget.yaml
262+ defaults:
263+ 2rGgvyaY0CCzlWuKAPwFtCWrgwkM8lqS:
264+ automatic-setup.disable: true
265+ EOF
266+ /snap/bin/se-test-tools.mksquashfs gadget $gadget_name.snap -comp xz -no-xattrs
267+ snap install --dangerous $gadget_name.snap
268+
269+ # Applying default configuration from a gadget snap only works when the
270+ # snap is installed and comes from the store. It's not possible for an
271+ # unasserted snap to be configured with defaults from the gadget.
272+ snap install --edge wifi-ap
273+
274+ test "$(snap get wifi-ap automatic-setup.disable)" = "true"
275diff --git a/tests/main/utf8-ssid/task.yaml b/tests/main/utf8-ssid/task.yaml
276index 30dcded..9b6faa6 100644
277--- a/tests/main/utf8-ssid/task.yaml
278+++ b/tests/main/utf8-ssid/task.yaml
279@@ -1,11 +1,17 @@
280 summary: Verify that the AP can accept an UTF8 SSID
281
282+environment:
283+ SCAN_ITERATIONS: 15
284+
285 execute: |
286 . $TESTSLIB/utilities.sh
287
288 # Default configuration will use wlan0 which we just created
289 /snap/bin/wifi-ap.config set 'wifi.ssid=Ubuntu👍' disabled=false
290
291+ # Ensure the wizard picked the right interface
292+ test "`/snap/bin/wifi-ap.config get 'wifi.interface'`" = "wlan0"
293+
294 # Wait for AP to become active
295 while ! /snap/bin/wifi-ap.status | grep 'ap.active: true' ; do
296 sleep 0.5
297@@ -19,7 +25,18 @@ execute: |
298 # Scan for existing WiFi networks and ensure our 'Ubuntu' one is part
299 # of the result
300 ifconfig wlan1 up
301- /snap/bin/wireless-tools.iw dev wlan1 scan | fgrep 'SSID: Ubuntu\xf0\x9f\x91\x8d'
302+ n=0
303+ found_ap=0
304+ while [ $n -lt $SCAN_ITERATIONS ] ; do
305+ /snap/bin/wireless-tools.iw dev wlan1 scan
306+ if /snap/bin/wireless-tools.iw dev wlan1 scan | fgrep 'SSID: Ubuntu\xf0\x9f\x91\x8d'; then
307+ found_ap=1
308+ break
309+ fi
310+ sleep 1
311+ n=$((n+1))
312+ done
313+ [ $found_ap -eq 1 ] || exit 1
314
315 # There should be only a single network
316 network_count=`/snap/bin/wireless-tools.iw dev wlan1 scan | fgrep -c 'SSID: Ubuntu\xf0\x9f\x91\x8d'`

Subscribers

People subscribed via source and target branches

to all changes: