Add support for snapd package manager #13
Conversation
noelmcloughlin
force-pushed
the
snapd_support
branch
6 times, most recently
from
cdb2434
to
5b4bcfe
Compare
noelmcloughlin
force-pushed
the
snapd_support
branch
from
5b4bcfe
to
4816786
Compare
|
Hey, @noelmcloughlin. Thanks for this PR. A question/issue: The idea with this formula is to be able to install/manage packages in a very basic way, so installing and maintaning a service (like I have never used snapd myself, so that's why I'd rather ask you 😄 : Am I right that this is like the latter? If that's the case, I'm OK with adding it here. If it requires/accept some sort of complex configuration (a la |
|
I never used snapd before today, but need this for kubernetes dev stuff. After reviewing the technology, and considering a separate formula briefly, it seemed more logical to add here. As separate formula, it would still work best as clone of packages-formula afaics. But reading the documentation, and using the tool, it feels like pip type solution. So it fits IMHO - separate formula feels like overkill. I'm trying to fix kitchen with systemd support. I don't think snaps SLS is optimal (i.e. depends on pkgs.sls) but future PR could fix? |
packages/defaults.yaml
Outdated
| @@ -21,3 +21,12 @@ packages: | |||
| required: | |||
| states: [] | |||
| pkgs: [] | |||
| snaps: | |||
| package: snapd | |||
| collides: ['snap',] | |||
There was a problem hiding this comment.
In Debian (and probably Ubuntu) is a gene manipulation software, not related to snap packages? Is this correct?
There was a problem hiding this comment.
On Fedora, snapd was conflicting with snap package? https://admin.fedoraproject.org/pkgdb/package/rpms/snap/
There was a problem hiding this comment.
Oh, I see. Debian's https://packages.debian.org/stretch/snap 😄
There was a problem hiding this comment.
Ha Ha. I thought your reference to gene manipulation was an allegory to something ...
There was a problem hiding this comment.
hehe, no, it's a 'real thing' 😄
packages.snaps.collides seems a candidate for osfamilymap.yml or osmap.yml, with an empty default, right?
There was a problem hiding this comment.
Does Default get matched in osmap.yaml? I was grepping inside a bunch of salt formulas trying to find example of usage but didn't and not sure. But if it is supported, absolutely yes.
There was a problem hiding this comment.
Check the map.jinja file: the order is to set defaults from defaults.yml (so 'yes', to your question), override with osfamilymap, then with osmap, then with pillar. Unless something's wrong with the map.jinja file, which is always possible 😄
There was a problem hiding this comment.
Yes, but I was wondering if following stanza works in osmap.yml? Will this match any OS not already listed?
...
Default:
snaps:
collides: ...
There was a problem hiding this comment.
No, not Default, but Fedora. It should be another part of the existing Fedora entry in osmap.yml, just like the existing one for pips. I think that will do.
There was a problem hiding this comment.
I tested on Ubuntu and no collision. Snap is not in OpenSUSE repo. snapd is not available for Centos yet.
Looks like Fedora only issue. I have updated the commit accordingly.
| extend: | ||
| unwanted_pkgs: | ||
| pkg.removed: | ||
| - pkgs: {{ packages.snaps.collides }} |
There was a problem hiding this comment.
As packages.snaps.collides is always set in defaults.yml, some packages will be always removed even if you don't want to manage snap packages. This will happen, at least with snap (see above)
Perhaps we can keep all these packages removals conditioned to packages.snaps.wanted != [] or packages.snaps.unwanted != []?
There was a problem hiding this comment.
I'm happy to change this - if you clearly see a nice solution - pls suggest.
So should I wrap extend inside jinja do you think? or maybe onlyif/unless.
(BTW: Normally extends goes to init.sls where includes are centralized but one thing at a time).
There was a problem hiding this comment.
onlyif/unless are 'cleaner' to me, but in this case, as there are 2 or'ed conditions and a few places where to use them, perhaps an if block surrounding all these will do for now?
There was a problem hiding this comment.
@javierbertoli Thanks for the review.
I agree. I've wrapped block with Jinja2 and I think the comparison operators are not needed - Lets see if Travis CI works and I test again on Fedora shortly.
There was a problem hiding this comment.
I tested packages and packages.pkgs, gems, pips (i.e. without snaps SLS) on Fedora and both high-states were successful.
There was a problem hiding this comment.
Travis CI was successful - Issue #14 looks like noise on Debian.
packages/snaps.sls
Outdated
| {% endfor %} | ||
| {% endif %} | ||
|
|
||
| {% for snap in packages.snaps.service %} |
There was a problem hiding this comment.
Same as above, perhaps we can install and manage the service conditioned to packages.snaps.wanted != [] or packages.snaps.unwanted != []?
noelmcloughlin
force-pushed
the
snapd_support
branch
2 times, most recently
from
2a0a380
to
ea94607
Compare
noelmcloughlin
force-pushed
the
snapd_support
branch
3 times, most recently
from
7092c45
to
c5f58e5
Compare
|
Debian is working now in kitchen test. This PR closes #14 |
noelmcloughlin
force-pushed
the
snapd_support
branch
3 times, most recently
from
e611c7c
to
1b5dae8
Compare
|
While using snap I found following issue on Fedora:
I updated PR to resolve this issue on Fedora only. |
|
BTW, reading this:
I have a silly sls to manage symlinks based on pillar data, pretty much the same way this formula does. Do you think it's worth to turn it into a formula? |
|
I'm not sure, maybe. To be honest I rarely need to create symlinks (except file.alternatives), but ask around to see if others are interested. BTW, I forked this repo while back, with intention of checking to see for potential formulas. He's handling some symlinks too ... |
|
Snap is working for me now on Fedora. Travis was stuck - I rebooted the job. |
noelmcloughlin
force-pushed
the
snapd_support
branch
2 times, most recently
from
7579f5f
to
a402ccc
Compare
|
Hi @javierbertoli I fixed kitchen.yml for Debian and Ubuntu so udev package gets installed. Fedora also works. There is some problem with Centos travis CI job which is unrelated to this PR. Could this be merged and centos kitchen issue be fixed separately? |
There was a problem hiding this comment.
@noelmcloughlin , would you agree to one last change and we can merge?
packages/defaults.yaml
Outdated
| package: snapd | ||
| collides: [] | ||
| symlink: False | ||
| service: ['snapd.service', 'snapd.socket',] |
There was a problem hiding this comment.
Mind you just change this line to
service: ['snapd']so the salt uses whatever the OS has (systemd, upstart, etc) and that's it? I checked and snapd.socket is a dependency of snapd.service, so there's no need to include it.
There was a problem hiding this comment.
thanks for suggestion, fixing that now.
noelmcloughlin
force-pushed
the
snapd_support
branch
from
a402ccc
to
57843d3
Compare
|
Hi @javierbertoli just one non-optimal implementation detail. Today, we use following pattern in pip / gems SLS. My PR follows the pip/gems pattern but introduces extends/include in snap SLS. I think this is fine for now. but overall not optimal. Other formulae seem to place extends/includes inside init.SLS. Just flagging this as maybe improvement needed? |
|
@noelmcloughlin submitted a PR to fix the CentOS issue. |
|
Thanks, starting Travis CI job again. Correction - seems to be running already. I created #15 for extends question. |
This PR adds formula support for snapd package manager.
Verified on Fedora: