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’ll 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
Merged

Add snap module #44939

merged 67 commits into from
Oct 17, 2018

Conversation

angristan
Copy link
Contributor

@angristan angristan 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 2 commits August 31, 2018 01:40
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
@ansibot ansibot added affects_2.7 This issue/PR affects Ansible v2.7 community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. new_contributor This PR is the first contribution by a new community member. new_module This PR includes a new module. new_plugin This PR includes a new plugin. support:community This issue/PR relates to code supported by the Ansible community. labels Aug 31, 2018
@mkrizek mkrizek removed the needs_triage Needs a first human triage before being processed. label Aug 31, 2018
@ansibot

This comment has been minimized.

@ansibot ansibot added ci_verified Changes made in this PR are causing tests to fail. needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Aug 31, 2018
@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Sep 2, 2018
@ansibot ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. labels Sep 2, 2018
@ansibot
Copy link
Contributor

ansibot 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

@ansibot ansibot removed the community_review In order to be merged, this PR must follow the community review workflow. label Sep 2, 2018
@angristan
Copy link
Contributor Author

line 188, in get_cmd_parts
TypeError: can only concatenate list (not "str") to list

@webknjaz
Copy link
Member

Ah.. right. Should be fixed now.

@angristan
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@webknjaz
Copy link
Member

Oh, it probably needs wrapping with sh -c

@webknjaz
Copy link
Member

test now

@angristan
Copy link
Contributor Author

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

It's working now 🎉

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job @angristan!

@angristan
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

OK, I will try to do that!

@benyanke
Copy link

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
Copy link
Member

@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
Copy link

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
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

Let's follow @webknjaz's advices 😄

@benyanke
Copy link

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

@assarbad
Copy link

assarbad 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 pushed a commit to Tomorrow9/ansible that referenced this pull request Dec 4, 2018
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 This PR modifies the BOTMETA.yml and this requires special attention! label Feb 21, 2019
@dagwieers dagwieers added the packaging Packaging category 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.
Labels
affects_2.7 This issue/PR affects Ansible v2.7 botmeta This PR modifies the BOTMETA.yml and this requires special attention! module This issue/PR relates to a module. needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. new_contributor This PR is the first contribution by a new community member. new_module This PR includes a new module. new_plugin This PR includes a new plugin. packaging Packaging category support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet