Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
cmd/snap-mgmt: introduce snap-mgmt tool #4316
Conversation
bboozzoo
added some commits
Nov 29, 2017
bboozzoo
referenced this pull request
Nov 29, 2017
Merged
packaging/arch: install snap-mgmt tool #4308
| + | ||
| + if test -d /etc/apparmor.d; then | ||
| + echo "Removing extra snap-confine apparmor rules" | ||
| + rm -f /etc/apparmor.d/snap.core.*.usr.lib.snapd.snap-confine |
zyga
Nov 29, 2017
•
Edited 1 time
-
zyga
Nov 29, 2017
Contributor
-
Nov 29, 2017
This location depends on $SNAP_MOUNT_DIR (i.e. snap is $(systemd-escape --path $SNAP_MOUNT_DIR)
bboozzoo
Nov 30, 2017
Contributor
The correct name is $(systemd-escape --path $SNAP_MOUNT_DIR).core.*.usr.lib.snapd.snap-confine?
bboozzoo
Nov 30, 2017
Contributor
On second thought, it's like: $(systemd-escape --path $SNAP_MOUNT_DIR | tr '-' '.').core.*.usr.lib.snapd.snap-confine
zyga
approved these changes
Nov 29, 2017
•
+1 assuming you take care of the thing I mentioned above ^ #4316 (review)
| + else | ||
| + # undo any bind mount to /snap that resulted from LP:#1668659 | ||
| + # (that bug can't happen in trusty -- and doing this would mess up snap.mount.service there) | ||
| + if grep -q "/snap /snap" /proc/self/mountinfo; then |
zyga
Nov 29, 2017
Contributor
This may be actually breaking containers. I will look at the LXD /snap rshare bug soon. Not a problem here, just an observation.
| @@ -518,9 +518,6 @@ rm %{buildroot}%{_libexecdir}/snapd/snapd.core-fixup.sh | ||
| # Disable re-exec by default | ||
| echo 'SNAP_REEXEC=0' > %{buildroot}%{_sysconfdir}/sysconfig/snapd | ||
| -# Install snap management script |
|
|
|
Don't merge this one yet, need a comment from @zyga |
|
Tests are unhappy with: |
bboozzoo
added some commits
Nov 30, 2017
codecov-io
commented
Nov 30, 2017
Codecov Report
@@ Coverage Diff @@
## master #4316 +/- ##
==========================================
+ Coverage 76.19% 77.92% +1.72%
==========================================
Files 445 445
Lines 38738 30818 -7920
==========================================
- Hits 29516 24014 -5502
+ Misses 7207 4794 -2413
+ Partials 2015 2010 -5
Continue to review full report at Codecov.
|
mvo5
merged commit f4b641c
into
snapcore:master
Dec 1, 2017
1 check passed
|
|
Conan-Kudo
reviewed
Dec 4, 2017
I'm incredibly disappointed that no one actually looked too carefully at this script when making it multi-distribution.
I'm also disappointed that no one pinged me for this review, because I would have liked to have given feedback before this was merged.
| - echo "Stoping $unit" | ||
| + for i in $(seq 10); do | ||
| + if systemctl is-active -q "$unit"; then | ||
| + echo "Stoping $unit" |
| + echo "Stoping $unit" | ||
| + systemctl stop -q "$unit" || true | ||
| + fi | ||
| + echo "Stoping $unit [attempt $i]" |
| systemctl stop -q "$unit" || true | ||
| - fi | ||
| + sleep .1 |
| - # undo any bind mount to ${SNAP_MOUNT_DIR} that resulted from LP:#1668659 | ||
| - if grep -q "${SNAP_MOUNT_DIR} ${SNAP_MOUNT_DIR}" /proc/self/mountinfo; then | ||
| - umount -l "${SNAP_MOUNT_DIR}" || true | ||
| + if grep -q CODENAME=trusty /etc/os-release >/dev/null 2>&1 ; then |
Conan-Kudo
Dec 4, 2017
•
Edited 1 time
-
Conan-Kudo
Dec 4, 2017
Contributor
-
Dec 4, 2017
Please don't do this. This is horribly brittle and broken. You have no idea what the content of this file is going to be, and you're hoping that this will work?
Also, you know os-release(5) is designed to be able to be sourced in shell, right? It works the same way as RH-style ifcfg files and any other key=value file.
| - if grep -q "${SNAP_MOUNT_DIR} ${SNAP_MOUNT_DIR}" /proc/self/mountinfo; then | ||
| - umount -l "${SNAP_MOUNT_DIR}" || true | ||
| + if grep -q CODENAME=trusty /etc/os-release >/dev/null 2>&1 ; then | ||
| + # snap.mount.service is a trusty thing |
Conan-Kudo
Dec 4, 2017
Contributor
Please do not refer to distribution releases by specific codenames, refer to them by their full names. Context is important for a multi-distribution script.
| + systemctl_stop snap.mount.service | ||
| + else | ||
| + # undo any bind mount to ${SNAP_MOUNT_DIR} that resulted from LP:#1668659 | ||
| + # (that bug can't happen in trusty -- and doing this would mess up snap.mount.service there) |
| + if test -d /etc/apparmor.d; then | ||
| + echo "Removing extra snap-confine apparmor rules" | ||
| + # shellcheck disable=SC2046 | ||
| + rm -f /etc/apparmor.d/$(echo "$SNAP_UNIT_PREFIX" | tr '-' '.').core.*.usr.lib.snapd.snap-confine |
Conan-Kudo
Dec 4, 2017
Contributor
You cannot guarantee the path for snap-confine is in fact /usr/lib/snapd. Please correct this appropriately.
bboozzoo
Dec 4, 2017
Contributor
AFAIU this is the path inside the core snap, not your host's $(libexecdir)/snapd/snap-confine
Conan-Kudo
Dec 4, 2017
Contributor
On all systems that aren't Ubuntu, they will be the host's. No one else has re-exec enabled.
| @@ -518,9 +518,6 @@ rm %{buildroot}%{_libexecdir}/snapd/snapd.core-fixup.sh | ||
| # Disable re-exec by default | ||
| echo 'SNAP_REEXEC=0' > %{buildroot}%{_sysconfdir}/sysconfig/snapd | ||
| -# Install snap management script |
|
@Conan-Kudo Thanks for reviewing this. I'll try to address the issues you raised in a PR. |
bboozzoo commentedNov 29, 2017
•
Edited 1 time
-
bboozzoo
Nov 29, 2017
Introduce
snap-mgmttool that is a merge ofpostrmactions in Ubuntu/Debian packages andsnap-mgmt.shshipped as part of Fedora package.This is related to #4308 where such change was proposed first.
If this is merged, then the next step would be to update distro packaging.