Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add snap module #44939

Merged
merged 67 commits into from Oct 17, 2018

Conversation

@angristan
Copy link
Contributor

commented Aug 31, 2018

SUMMARY

This PR adds the snap module to manage snaps packages (see snapcraft.io).

Features:

It is based on the PR #40852 and the work made by @vcarceler. (Thanks a bunch!)
His branch runs against a number of issues against the CI and he has not made any update in 3 months, so I'm finishing the work. In the end, I did a bit of refactoring 馃槃

This fixes issue #39155.

  • All sanity tests are now successful (and the module actually works! 馃帀)
  • I improved the module documentation
  • I removed a lot of code and comments that were just a copy/paste from the ansible doc's sample module. Most of it was unneeded. Some just broked the code.
  • I refactored the code:
    • It's easier to read and understand
    • It's shorter
    • Functions make more sense
    • Some bugs have been fixed, like the output on exit_json and fail_json

I would like to implement more features like snap refresh, but sadly I don't think this is feasible in a clean way for now, considering how the cli works.

Any feedback is welcome! I'm new to this.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME
snap

More specifically:

packaging/os/snap
ANSIBLE VERSION
$ ansible --version
ansible 2.7.0.a1.post0 (snapcraft fc4e1c9b46) last updated 2018/08/30 21:27:31 (GMT +200)
  config file = None
  configured module search path = ['/Users/stanislas/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /Users/stanislas/lab/ansible/lib/ansible
  executable location = /Users/stanislas/lab/ansible/bin/ansible
  python version = 3.7.0 (default, Jul  7 2018, 00:33:58) [Clang 9.1.0 (clang-902.0.39.2)]

vcarceler and others added some commits May 29, 2018

Refactor snap module
First, thanks to Victor Carceler for the base module.

* All sanity tests are now successful
* Improved module doc
* Removed a lot of code and comments that were just a copy/paste from the ansible doc:
  https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_general.html#new*module*development
  Most of it was uneeded. Some just broked the code.
* Refactored the code:
  * It's easier to read and understand
  * It's shorter
  * functions make more sense
  * Some bugs have been fixed, like the output on exit_json and fail_json

@mkrizek mkrizek removed the needs_triage label Aug 31, 2018

@ansibot

This comment was marked as outdated.

Copy link
Contributor

commented Aug 31, 2018

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/packaging/os/snap.py:0:0: E307 version_added should be 2.7. Currently 2.8

click here for bot help

@ansibot ansibot removed the ci_verified label Sep 2, 2018

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2018

@angristan This PR contains @ mentions in at least one commit message. Those mentions can cause cascading notifications through GitHub and need to be removed. Please squash or amend your commits to remove the mentions.

click here for bot help

@webknjaz

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Ah.. right. Should be fixed now.

@angristan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

Indeed 馃憤

I'm encountering a weird bug right now...

fatal: [root@server]: FAILED! => {
    "changed": false,
    "channel": "stable",
    "classic": true,
    "cmd": "/usr/bin/snap install --classic vlc; /usr/bin/snap install --classic vault",
    "invocation": {
        "module_args": {
            "channel": "stable",
            "classic": true,
            "name": [
                "vlc",
                "vault"
            ],
            "state": "present"
        }
    },
    "msg": "Ooops! Snap installation failed while executing '/usr/bin/snap install --classic vlc; /usr/bin/snap install --classic vault', please examine logs and error output for more details.",
    "rc": 1,
    "stderr": "error: a single snap name is needed to specify mode or channel flags\n",
    "stderr_lines": [
        "error: a single snap name is needed to specify mode or channel flags"
    ],
    "stdout": "",
    "stdout_lines": []
}

On the server:

root@snap:~# /usr/bin/snap install --classic vlc; /usr/bin/snap install --classic vault
vlc 3.0.4 from VideoLAN鉁 installed
vault 0.11.3 from Snapcrafters installed
@angristan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

Unless the cmd output is different from what's actually executed, I don't understand what's wrong 馃

@webknjaz

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

Oh, it probably needs wrapping with sh -c

@webknjaz

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

test now

@angristan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

oooh ok, since ; is shell, it makes sense.

It's working now 馃帀

@webknjaz
Copy link
Member

left a comment

Good job @angristan!

@angristan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

Thank you very, very much! It was a great experience and I appreciate you taking the time (and having the patience) to help a beginner. 馃檱

@webknjaz webknjaz merged commit 9cc9ca5 into ansible:devel Oct 17, 2018

1 check passed

Shippable Run 89002 status is SUCCESS.
Details
@webknjaz

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

@angristan you are welcome :)

P.S. Feel free to send follow-up PRs adding regression tests. Here's some example of how it's done for other modules https://github.com/ansible/ansible/blob/devel/test/integration/targets/apt. Essentially it's just playbooks running the module and then asserting results. Seek more info in docs :)

@angristan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2018

OK, I will try to do that!

@benyanke

This comment has been minimized.

Copy link

commented Oct 19, 2018

As someone looking in from the outside a bit, I'm quite excited to see this. When is this expected to hit a stable ansible release, and is there any way of using it sooner without wholesale installing ansible from git?

@webknjaz

This comment has been minimized.

Copy link
Member

commented Oct 21, 2018

@benyanke wait for 2.8 or copy-paste this module into a library/ folder next to your playbooks.

Ref: https://docs.ansible.com/ansible/devel/user_guide/playbooks_best_practices.html#bundling-ansible-modules-with-playbooks

@benyanke

This comment has been minimized.

Copy link

commented Oct 23, 2018

I'm trying this out and it's working well overall, but it fails on snap install hooks the first time, then succeeds if run again. Should I document here or make a new issue?

@angristan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

Thanks for reporting @benyanke, since the module is still on devel for now, I think you can post here. Please reproduce the issue while running ansible with the -vvv flag.

@webknjaz

This comment has been minimized.

Copy link
Member

commented Oct 23, 2018

@benyanke in general it's better to open new issues and PRs, because closed/merged things don't get much attention and get lost.
Also, our bot won't be able to help classifying issue with labels or pinging maintainers. Moreover, dumping a bunch of different issues into one ticket creates spaghetti, which is hard to get through.

Rule of thumb 馃憤: create one separate issue or/and PR per problem/request. Also, @angristan has power to +1 PRs sent against this module in case you'll decide to send a patch.

@angristan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 23, 2018

Let's follow @webknjaz's advices 馃槃

@benyanke

This comment has been minimized.

Copy link

commented Oct 23, 2018

Sounds good to me. Will open a new ticket (and reference this one) when I have a chance to replicate it

@assarbad

This comment has been minimized.

Copy link

commented Nov 2, 2018

Something that would be cool for this module would be if it had something like state: latest, i.e. that if the snap package is already installed it attempts snap refresh on it.

Thanks for the module. I am now using it with Ansible 2.7.1 as pointed out above.

Tomorrow9 added a commit to Tomorrow9/ansible that referenced this pull request Dec 4, 2018

Implement snap packaging module
PR ansible#44939

Fixes ansible#39155
Closes ansible#40852

Co-authored-by: Victor Carceler <vcarceler@iespuigcastellar.xeill.net>
Co-authored-by: Stanislas Lange <angristan@pm.me>
Co-authored-by: Sviatoslav Sydorenko <wk@sydorenko.org.ua>

@dagwieers dagwieers added the botmeta label Feb 21, 2019

@dagwieers dagwieers added the packaging label Mar 3, 2019

@ansible ansible locked and limited conversation to collaborators Jul 22, 2019

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can鈥檛 perform that action at this time.