Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
many: support debian in our CI #3156
Conversation
| @@ -54,7 +54,7 @@ ifeq ($(shell dpkg-vendor --query Vendor),Ubuntu) | ||
| # libudev and udevd so we need to link with it dynamically. | ||
| VENDOR_ARGS=--enable-nvidia-ubuntu --enable-static-libcap --enable-static-libapparmor --enable-static-libseccomp | ||
| else | ||
| - VENDOR_ARGS=--disable-apparmor | ||
| + VENDOR_ARGS=--disable-apparmor --disable-seccomp |
zyga
Apr 10, 2017
Contributor
Can you do a second ifeq and check for Debian, the else part should be empty.
morphis
Apr 19, 2017
Contributor
@zyga You mean a second ifeq within the else part or dropping the else part completely?
zyga
Apr 25, 2017
Contributor
A second, chained ifeq so that the else is applied to !ubuntu and !debian
| @@ -17,7 +17,7 @@ data/udev/rules.d/66-snapd-autoimport.rules /lib/udev/rules.d | ||
| data/info /usr/lib/snapd/ | ||
| # snap-confine stuff | ||
| -etc/apparmor.d/usr.lib.snapd.snap-confine.real | ||
| +# etc/apparmor.d/usr.lib.snapd.snap-confine.real |
zyga
Apr 10, 2017
Contributor
Perhaps we should remove the top-level symlink and just add packaging/debian-9 and let the CI part add the relevant stuff. Note that for fedora the process is totally different so we need to re-think how this is done.
| @@ -54,6 +54,9 @@ backends: | ||
| kernel: Direct Disk | ||
| image: ubuntu-16.04-64 | ||
| workers: 2 | ||
| + - debian-9-64: | ||
| + kernel: GRUB 2 |
morphis
changed the title from
WIP: debian support for spread testing
to
Support debian in our CI
Apr 19, 2017
morphis
changed the title from
Support debian in our CI
to
many: support debian in our CI
Apr 19, 2017
| +func init() { | ||
| + cmd := addCommand("is-forced-devmode", | ||
| + i18n.G("Check if forced devmode is enabled or not"), | ||
| + i18n.G("Check if forced devmode is enabled or not"), |
zyga
Apr 19, 2017
Contributor
I suspect this part doesn't respect the rules for command help and long description. Have a look at other commands and what they do.
| @@ -1,7 +1,7 @@ | ||
| summary: Check different completions | ||
| # ppc64el disabled because of https://github.com/snapcore/snapd/issues/2502 | ||
| -systems: [-ubuntu-core-16-*, -ubuntu-*-ppc64el] | ||
| +systems: [-ubuntu-core-16-*, -ubuntu-*-ppc64el, -debian-*] |
morphis
Apr 19, 2017
Contributor
As commit message says it's somehow failing as the expected output is different. Something I am still looking into.
| @@ -1,5 +1,7 @@ | ||
| summary: Checks for snap sideload install | ||
| +systems: [-debian-*] |
zyga
Apr 19, 2017
Contributor
Please always indicate why something is disabled on any given system. Otherwise it is just a bug waiting to be discovered.
| -systems: [-ubuntu-core-16-*,-debian-*] | ||
| +systems: | ||
| + - -ubuntu-core-16 | ||
| + # No confinement (AppArmor, Seccomp) available on these systems |
zyga
Apr 19, 2017
Contributor
But this is classic confinement :) It should just work, except for jailmode. I think we should not disable any such test but instead make the part that exercises confinement dependant on the hidden command that tells us if confinement is enabled or not.
morphis
Apr 20, 2017
Contributor
Exactly that is the plan but in a second wave. As soon as all tests are green I want a first version landed. Using a snap is-forced-devmode check will then require some refactoring in the tests which I want clearly separate from this to not make the PRs bigger as needed.
This is classic confinement but fails because of jailmode. Lets enable this again in the second wave. Its not like I am throwing a PR and will never care again :-)
|
There are still a few failures in tests, do you want to get a full review of the branch before those go green? |
|
@zyga If you want, please shoot your comments. I am still working on the remaining test cases but another review should be fine already at this stage. |
| + # FIXME jailmode doesn't work on debian type systems due to missing | ||
| + # confinement yet. This check will be replaced with proper detection | ||
| + # if confinement is supported on the platform we're running. | ||
| + if [ "$SPREAD_SYSTEM" != "debian-*" ]; then |
morphis
Apr 25, 2017
Contributor
Yeah, thanks for spotting this. One I missed after I found this out myself.
mvo5
reviewed
Apr 26, 2017
Looks good, thanks for doing this work. I'm impressed to see that Debian is now actually passing! Some small suggestion inline but I think this is very close.
| @@ -1,7 +1,9 @@ | ||
| summary: Check different completions | ||
| -# ppc64el disabled because of https://github.com/snapcore/snapd/issues/2502 | ||
| -systems: [-ubuntu-core-16-*, -ubuntu-*-ppc64el] | ||
| +systems: |
mvo5
Apr 26, 2017
Collaborator
(nitpick) this change looks unrelated. Fine to have it, just point this out. I guess initially debian was disabled here?
morphis
Apr 26, 2017
Contributor
Ok, I remember my intention for this. The comment above only applies for ppc so I moved this to an expanded yaml array to put the comment directly above the ppc line.
| @@ -68,6 +68,13 @@ execute: | | ||
| echo "And plug is disconnected by default" | ||
| snap interfaces | MATCH "$DISCONNECTED_PATTERN" | ||
| + # FIXME This is a pretty bad check for unsupported confinement |
mvo5
Apr 26, 2017
Collaborator
I wonder if we should just skip the entire test on debian (avoiding this slightly ugly thing in the middle).
morphis
Apr 26, 2017
Contributor
I would like to run the first part of this as it exercise the dbus service setup through the configuration in the snap.yaml. snapd still puts the dbus configuration file in place to allow ownership over bus names which will be still validated if we have snap confinement or not.
| - echo Hello > /var/tmp/myevil.txt | ||
| - test-snapd-tools.cat /var/tmp/myevil.txt && exit 1 || true | ||
| + | ||
| + if [ -e /sys/kernel/security/apparmor ]; then |
mvo5
Apr 26, 2017
Collaborator
Maybe a small comment like: only test if confinement works if we actually have apparmor available ?
|
I think you need to disable one more test: This is on the openvswitch interface test. |
|
Looks like that one was added in between. |
| - make -C test-snapd-hello-classic clean | ||
| - make -C test-snapd-hello-classic | ||
| - snap install $INSTALL_FLAGS --dangerous ./test-snapd-hello-classic/test-snapd-hello-classic_0.1_*.snap | ||
| + run_install() { |
zyga
Apr 26, 2017
Contributor
I don't get one thing. The environment section above spawns two instances of this test, each with its own prepare code. Now you roll this into one where we run_install --classic and run_install --jailmode in one prepare step. This effectively means that we no longer test --classic sans --jailmode. Am I missing anything?
morphis
Apr 27, 2017
•
Edited 1 time
-
morphis
Apr 27, 2017
Contributor
-
Apr 27, 2017
Will see how I can workaround that.
| - test-snapd-tools.cat /var/tmp/myevil.txt && exit 1 || true | ||
| + | ||
| + # only test if confinement works and we actually have apparmor available | ||
| + if [ -e /sys/kernel/security/apparmor ]; then |
zyga
Apr 26, 2017
Contributor
Can you please add a note that says this should migrate to the internal command that queries forced devmode.
morphis commentedApr 10, 2017
•
Edited 1 time
-
morphis
Apr 20, 2017
This adds support for debian based images on Linode to our CI infrastructure.