many: add magic /snap/README file #4210

Merged
merged 8 commits into from Nov 15, 2017

Conversation

Projects
None yet
6 participants
Contributor

zyga commented Nov 13, 2017

This branch makes snapd manage and maintain a $SNAP_MOUNT_DIR/README file
(typically /snap/README) with helpful text about the purpose of that
directory.

The text should be internationalized (a few common variants should be added).
In addition there is a link to the snapcraft forum with a thread containing
extended documentation.

zyga added some commits Nov 13, 2017

overlord,tests: maintain a README file in /snap
This patch changes the snap manager to maintain a readme file in the
/snap directory. The file contains a piece of curated text and a
reference to the forum thread documenting the purpose of the directory.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
overlord/snapstate: use EnsureDirState to preserve media
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

Looks reasonable. How about this for the wording:

overlord/snapstate/readme.go
+files inside may seem large almost no space is consumed here. The actual space
+is used by heavily-compressed .snap files stored in /var/lib/snapd/snap
+
+For more information please visit: https://forum.snapcraft.io/t/the-snap-directory/
@niemeyer

niemeyer Nov 13, 2017

Contributor

How about something like this:

This directory presents installed snap packages. 

It has the following structure:

/snap/bin                   - Symlinks to snap applications.
/snap/<snapname>/<revision> - Mountpoint for snap content.
/snap/<snapname>/current    - Symlink to current revision, if enabled.

DISK SPACE USAGE

The disk space consumed by the content under this directory is
minimal as the real snap content never leaves the .snap file.
Snaps are *mounted* rather than unpacked.

For further details please visit
https://forum.snapcraft.io/t/the-snap-directory/<topic id>

Then, let's please have a topic so that we can include the topic ID in the link as well, so that if it ever moves or gets renamed the link continues to work.

@zyga

zyga Nov 13, 2017

Contributor

The topic is already live: https://forum.snapcraft.io/t/the-snap-directory/2817 (the link I used was real :-)

@zyga

zyga Nov 13, 2017

Contributor

Ah, I understand the remark now, done.

codecov-io commented Nov 13, 2017

Codecov Report

Merging #4210 into master will decrease coverage by <.01%.
The diff coverage is 78.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4210      +/-   ##
==========================================
- Coverage   75.83%   75.83%   -0.01%     
==========================================
  Files         439      440       +1     
  Lines       38176    38190      +14     
==========================================
+ Hits        28952    28962      +10     
- Misses       7214     7217       +3     
- Partials     2010     2011       +1
Impacted Files Coverage Δ
overlord/snapstate/snapmgr.go 84.08% <100%> (+0.07%) ⬆️
overlord/snapstate/readme.go 75% <75%> (ø)
overlord/ifacestate/helpers.go 59.6% <0%> (-0.67%) ⬇️
cmd/snap/cmd_aliases.go 95% <0%> (+1.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 960abc1...05a03c4. Read the comment docs.

mvo5 approved these changes Nov 14, 2017

Nice touch, thank you.

zyga added some commits Nov 13, 2017

packaging: handle the /snap/README file
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
many: adjusts text in /snap/README
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Member

chipaca commented Nov 14, 2017

On the one hand I like this. On the other, and I'm not sure how bikesheddy this is, this is very sysadmin-unfriendly: not only are we overwriting any change a sysadmin does to the file (think: translating, adding local notes), we're also not saying so; the file also appears out of thin air.

So, first, if the sysadmin isn't supposed to edit the file, at least make it 0444 (most editors will then warn). Ideally also use osutil.SetAttr to make it immutable.

I'd be happier if the file existed somewhere on disk and what we did was symlink or copy it here, instead of holding it in memory, but I can see how that's harder to do.

I think shipping it translated is a bit of a lost cause; unless each user sees it in their preferred locale it's not a net win (and plenty of system-level files are already single-locale and usually English-only, so it's not like we're breaking new anti-l10n ground here).

Manual-like readme files should be fully explanatory (i.e. like a man page) and not require someone to go to a website.

Secondly, the path should be dynamically filled in based on what the expected SNAP_MOUNT_DIR will be. It's certainly not /snap in Fedora and Arch, for example.

overlord/snapstate: make README file 0444
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Contributor

Conan-Kudo commented Nov 14, 2017

@zyga Making it 0444 means that snapd wouldn't be able to update it anymore after creating it, ne?

overlord/snapstate: tailor README for distribution
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Contributor

zyga commented Nov 14, 2017

@Conan-Kudo I fixed that, thank you for noticing!

overlord/snapstate/readme.go
-/snap/<snapname>/<revision> - Mountpoint for snap content.
-/snap/<snapname>/current - Symlink to current revision, if enabled.
+@SNAP_MOUNT_DIR@/bin - Symlinks to snap applications.
+@SNAP_MOUNT_DIR@/<snapname>/<revision>- Mountpoint for snap content.
@niemeyer

niemeyer Nov 14, 2017

Contributor

Spacing is now broken here. (>-),

@zyga

zyga Nov 14, 2017

Contributor

Fixed

overlord/snapstate: fix off-by-one space
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

@mvo5 mvo5 merged commit 81fa7c8 into snapcore:master Nov 15, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment