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 matrix notification module #45823

Merged
merged 10 commits into from Oct 24, 2018

Conversation

Projects
None yet
7 participants
@jcgruenhage
Copy link
Contributor

commented Sep 18, 2018

SUMMARY

This is a matrix notification module, to live alongside telegram, slack and the rest of the crowd.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

matrix

ANSIBLE VERSION
ansible 2.8.0.dev0 (matrix-notification-module bd6dec4406) last updated 2018/09/19 00:48:30 (GMT +200)
  config file = /etc/ansible/ansible.cfg
  configured module search path = ['/home/jcgruenhage/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /home/jcgruenhage/dev/ansible/lib/ansible
  executable location = /home/jcgruenhage/dev/ansible/bin/ansible
  python version = 3.6.6 (default, Jul 19 2018, 14:25:17) [GCC 8.1.1 20180712 (Red Hat 8.1.1-5)]
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2018

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

lib/ansible/modules/notification/matrix.py:114:0: trailing-whitespace Trailing whitespace

The test ansible-test sanity --test boilerplate [explain] failed with 2 errors:

lib/ansible/modules/notification/matrix.py:0:0: missing: __metaclass__ = type
lib/ansible/modules/notification/matrix.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/notification/matrix.py:49:0: SyntaxError: Non-ASCII character '\xc3' in file /root/ansible/lib/ansible/modules/notification/matrix.py on line 50, but no encoding declared; see http://www.python.org/peps/pep-0263.html for details

The test ansible-test sanity --test import --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/notification/matrix.py:49:0: SyntaxError: Non-ASCII character '\xc3' in file /root/ansible/lib/ansible/modules/notification/matrix.py on line 50, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details

The test ansible-test sanity --test import --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/notification/matrix.py:75:0: ImportError: No module named 'matrix_client'

The test ansible-test sanity --test import --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/notification/matrix.py:75:0: ModuleNotFoundError: No module named 'matrix_client'

The test ansible-test sanity --test import --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/notification/matrix.py:75:0: ModuleNotFoundError: No module named 'matrix_client'

The test ansible-test sanity --test pep8 [explain] failed with 7 errors:

lib/ansible/modules/notification/matrix.py:46:1: W293 blank line contains whitespace
lib/ansible/modules/notification/matrix.py:60:1: W293 blank line contains whitespace
lib/ansible/modules/notification/matrix.py:68:43: W291 trailing whitespace
lib/ansible/modules/notification/matrix.py:77:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/notification/matrix.py:114:1: W293 blank line contains whitespace
lib/ansible/modules/notification/matrix.py:117:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/notification/matrix.py:120:1: E305 expected 2 blank lines after class or function definition, found 1

The test ansible-test sanity --test validate-modules [explain] failed with 3 errors:

lib/ansible/modules/notification/matrix.py:0:0: E105 GPLv3 license header not found in the first 20 lines of the module
lib/ansible/modules/notification/matrix.py:0:0: E307 version_added should be 2.8. Currently 2.7
lib/ansible/modules/notification/matrix.py:0:0: E321 Exception attempting to import module for argument_spec introspection, 'No module named 'matrix_client''

click here for bot help

@jcgruenhage

This comment has been minimized.

Copy link
Contributor Author

commented Sep 19, 2018

Looks like I've still got some work to do :D

@ansibot ansibot removed the ci_verified label Sep 19, 2018

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2018

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/notification/matrix.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/notification/matrix.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/notification/matrix.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/notification/matrix.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/notification/matrix.py:0:0: has a documentation error formatting or is missing documentation.

The test ansible-test sanity --test docs-build [explain] failed with the error:

Command "/usr/bin/python test/sanity/code-smell/docs-build.py" returned exit status 1.
>>> Standard Error
Traceback (most recent call last):
  File "test/sanity/code-smell/docs-build.py", line 99, in <module>
    main()
  File "test/sanity/code-smell/docs-build.py", line 17, in main
    raise subprocess.CalledProcessError(sphinx.returncode, cmd, output=stdout, stderr=stderr)
subprocess.CalledProcessError: Command '['make', 'singlehtmldocs']' returned non-zero exit status 2.

The test ansible-test sanity --test boilerplate [explain] failed with 2 errors:

lib/ansible/modules/notification/matrix.py:0:0: missing: __metaclass__ = type
lib/ansible/modules/notification/matrix.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

click here for bot help

@jborean93

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

@jcgruenhage it is a lot easier and quicker to run the sanity tests locally. This saves you waiting for Shippable to run and you can run individual sanity tests instead of the whole bunch. To do this you can run

ansible-test sanity --docker default --test <test here>

You can double up on tests by appending more --test <another test> --test <one more test> to the command. For example if you wanted to run ansible-doc, docs-build and boilerplate locally you can do

ansible-test sanity --docker default --test ansible-doc --test docs-build --test boilerplate
@jcgruenhage

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2018

Oh cool, thanks 👍

@jcgruenhage

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2018

@jborean93 In what directory are these supposed to be run? If run in the repository root I get a bunch of things for packages in the venv and in the directory of the module it fails to run..

@jborean93

This comment has been minimized.

Copy link
Contributor

commented Sep 20, 2018

@jcgruenhage it's best to run source hacking/env-setup in your Ansible git checkout to ensure you are using the Ansible test for the current code. If you are getting errors still, can you share the output and we can help.

@jcgruenhage

This comment has been minimized.

Copy link
Contributor Author

commented Sep 21, 2018

I did that. In the root of the repository I get a bunch of these:

ERROR: venv/lib/python2.7/site-packages/yaml/emitter.py:0:0: missing: __metaclass__ = type
ERROR: venv/lib/python2.7/site-packages/yaml/emitter.py:0:0: missing: from __future__ import (absolute_import, division, print_function)

They clearly don't matter, because it's venv packages and not what I am contributing.

If I run it in the directory of the module I'm contributing, I get this:

 jcgruenhage  (e) venv  …  ansible  modules  notification  ansible-test sanity --docker default --test boilerplate
Sending build context to Docker daemon  189.4kB
Step 1/8 : FROM debian:7.7
 ---> 6d129b37ec3b
Step 2/8 : MAINTAINER Rob McQueen
 ---> Using cache
 ---> 4de027bf81db
Step 3/8 : RUN apt-get update && apt-get install -y   python-dev   python-virtualenv   sudo
 ---> Using cache
 ---> b9e3a3bd6be6
Step 4/8 : RUN mkdir /opt/ansible && virtualenv /opt/ansible/venv &&   /opt/ansible/venv/bin/pip install ansible
 ---> Running in bcdac859859e
New python executable in /opt/ansible/venv/bin/python
Installing distribute............................................................................................................
.................................................................................done.
Installing pip...............done.
Downloading/unpacking ansible
  Cannot fetch index base URL http://pypi.python.org/simple/
  Could not find any downloads that satisfy the requirement ansible
No distributions at all found for ansible
Storing complete log in /root/.pip/pip.log
The command '/bin/sh -c mkdir /opt/ansible && virtualenv /opt/ansible/venv &&   /opt/ansible/venv/bin/pip install ansible' returned a non-zero code: 1
Traceback (most recent call last):
  File "/usr/bin/ansible-test", line 69, in <module>
    main(context)
  File "/usr/bin/ansible-test", line 45, in main
    '-t', DOCKER_CONTAINER, '.']
  File "/usr/lib64/python2.7/subprocess.py", line 190, in check_call
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['docker', 'build', '-t', 'testbox', '.']' returned non-zero exit status 1

@ansibot ansibot added the stale_ci label Sep 29, 2018

jcgruenhage added some commits Sep 29, 2018

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2018

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

lib/ansible/modules/notification/matrix.py:74:0: misplaced-future __future__ import is not the first non docstring statement

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/notification/matrix.py:74:0: SyntaxError: from __future__ imports must occur at the beginning of the file

The test ansible-test sanity --test import --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/notification/matrix.py:74:0: SyntaxError: from __future__ imports must occur at the beginning of the file

The test ansible-test sanity --test import --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/notification/matrix.py:74:0: SyntaxError: from __future__ imports must occur at the beginning of the file

The test ansible-test sanity --test import --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/notification/matrix.py:74:0: SyntaxError: from __future__ imports must occur at the beginning of the file

The test ansible-test sanity --test import --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/notification/matrix.py:74:0: SyntaxError: from __future__ imports must occur at the beginning of the file

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/notification/matrix.py:48:7: W291 trailing whitespace
lib/ansible/modules/notification/matrix.py:130:5: E265 block comment should start with '# '

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

lib/ansible/modules/notification/matrix.py:0:0: E321 Exception attempting to import module for argument_spec introspection, 'from __future__ imports must occur at the beginning of the file (matrix.py, line 74)'

click here for bot help

@ansibot ansibot added the ci_verified label Sep 29, 2018

@ansibot ansibot removed the ci_verified label Sep 29, 2018

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2018

@Deepakkothandan @Jmainguy @bjolivot @bkimble @drew-russell @fabulops @garbled1 @jcftang @jpmens @makaimc @mcodd @mpdehaan @pb8226 @shirou @tksmd @tonyseek @tyouxa @weaselkeeper @willybarro @zimbatm

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@jcgruenhage

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

It's been a while, is there anything more I can do aside from sending a "bump" comment every now and then?

@Jmainguy

This comment has been minimized.

Copy link
Contributor

commented Oct 17, 2018

I have found asking for a review in #ansible-devel on the Freenode IRC has been helpful in moving things along. I don't have a matrix account or time to test this right now, but someone else in that channel likely will.

@jcgruenhage

This comment has been minimized.

Copy link
Contributor Author

commented Oct 17, 2018

I'll try that, thanks

msg_html:
description:
- HTML form of the message to send to matrix
required: true

This comment has been minimized.

Copy link
@bcoca

bcoca Oct 17, 2018

Member

i would have single 'msg' and then a 'type' option, this follows the pattern of other modules.

This comment has been minimized.

Copy link
@jcgruenhage

jcgruenhage Oct 17, 2018

Author Contributor

That would be fit if the message sent contains either a plain text or an html message. Matrix sends both, a plain text variant (usually markdown) and also an HTML variant in the content of a text message event, unless there is no formatting (then the HTML variant is omitted). Because of that, the two different args are required as is.

description:
- The password to log in with
notes:
- Requires matrix-client on the executing host.

This comment has been minimized.

Copy link
@bcoca

bcoca Oct 17, 2018

Member

add 'python library' , then the 'pip install' becomes redundant, some people cannot use pip to install libraries

This comment has been minimized.

Copy link
@jcgruenhage

jcgruenhage Oct 17, 2018

Author Contributor

This was adapted from pushbullet.py, but you're concern is definitely valid, will change it.

@ansibot ansibot removed the stale_ci label Oct 17, 2018

Example formatting & no_log
Thanks for this PR.
Few minor things that are easier for me to just fix, than explain and get you to fix.

* We suggest using `- name:` for examples, as Ansible best practice is to name your tasks
* To prevent secrets being leaked out use `no_log` in argspec
* use `requirements:` in `DOCUMENTATION`
@ansibot

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

The test ansible-test sanity --test ansible-doc --python 2.6 [explain] failed with the error:

Output on stderr from ansible-doc is considered an error.

Command "ansible-doc -t module matrix" returned exit status 0.
>>> Standard Error
[WARNING]: While constructing a mapping from
/root/ansible/lib/ansible/modules/notification/matrix.py, line 3, column 1,
found a duplicate dict key (requirements). Using last defined value only.

The test ansible-test sanity --test ansible-doc --python 2.7 [explain] failed with the error:

Output on stderr from ansible-doc is considered an error.

Command "ansible-doc -t module matrix" returned exit status 0.
>>> Standard Error
[WARNING]: While constructing a mapping from
/root/ansible/lib/ansible/modules/notification/matrix.py, line 3, column 1,
found a duplicate dict key (requirements). Using last defined value only.

The test ansible-test sanity --test ansible-doc --python 3.5 [explain] failed with the error:

Output on stderr from ansible-doc is considered an error.

Command "ansible-doc -t module matrix" returned exit status 0.
>>> Standard Error
[WARNING]: While constructing a mapping from
/root/ansible/lib/ansible/modules/notification/matrix.py, line 3, column 1,
found a duplicate dict key (requirements). Using last defined value only.

The test ansible-test sanity --test ansible-doc --python 3.6 [explain] failed with the error:

Output on stderr from ansible-doc is considered an error.

Command "ansible-doc -t module matrix" returned exit status 0.
>>> Standard Error
[WARNING]: While constructing a mapping from
/root/ansible/lib/ansible/modules/notification/matrix.py, line 3, column 1,
found a duplicate dict key (requirements). Using last defined value only.

The test ansible-test sanity --test ansible-doc --python 3.7 [explain] failed with the error:

Output on stderr from ansible-doc is considered an error.

Command "ansible-doc -t module matrix" returned exit status 0.
>>> Standard Error
[WARNING]: While constructing a mapping from
/root/ansible/lib/ansible/modules/notification/matrix.py, line 3, column 1,
found a duplicate dict key (requirements). Using last defined value only.

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

lib/ansible/modules/notification/matrix.py:51:1: key-duplicates DOCUMENTATION: duplication of key "requirements" in mapping

click here for bot help

jcgruenhage added some commits Oct 23, 2018

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

The test ansible-test sanity --test validate-modules [explain] failed with 4 errors:

lib/ansible/modules/notification/matrix.py:0:0: E332 AnsibleModule.mutually_exclusive.0: expected list @ data['mutually_exclusive'][0]. Got 'password'
lib/ansible/modules/notification/matrix.py:0:0: E332 AnsibleModule.mutually_exclusive.1: expected list @ data['mutually_exclusive'][1]. Got 'token'
lib/ansible/modules/notification/matrix.py:0:0: E332 AnsibleModule.required_together.0: expected list @ data['required_together'][0]. Got 'user_id'
lib/ansible/modules/notification/matrix.py:0:0: E332 AnsibleModule.required_together.1: expected list @ data['required_together'][1]. Got 'password'

click here for bot help

@ansibot ansibot added the ci_verified label Oct 23, 2018

@ansibot ansibot removed the ci_verified label Oct 23, 2018

@gundalow
Copy link
Contributor

left a comment

Looks great, thank you for the prompt fixes

@gundalow gundalow merged commit 01bee0c into ansible:devel Oct 24, 2018

1 check passed

Shippable Run 90038 status is SUCCESS.
Details
@gundalow

This comment has been minimized.

Copy link
Contributor

commented Oct 24, 2018

Thank you for the new module. It's been merged into devel, and will be released in Ansible 2.8.

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

Add matrix notification module (ansible#45823)
* Add matrix notification module

* try to make ansibot happy

* docs

* fix typo in encoding

* is ansibot happy now?

* change matrix python lib requirement description

* Example formatting & no_log

Thanks for this PR.
Few minor things that are easier for me to just fix, than explain and get you to fix.

* We suggest using `- name:` for examples, as Ansible best practice is to name your tasks
* To prevent secrets being leaked out use `no_log` in argspec
* use `requirements:` in `DOCUMENTATION`

* Clean up argument requirements

* Remove requirements duplicate

* not sure on syntax with these, were adapted from an example elsewhere
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.