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

RabbitMQ publisher module #44718

Merged
merged 23 commits into from
Nov 15, 2018
Merged

RabbitMQ publisher module #44718

merged 23 commits into from
Nov 15, 2018

Conversation

Im0
Copy link
Contributor

@Im0 Im0 commented Aug 27, 2018

SUMMARY

Publish a message on a rabbitmq queue using a basic publisher

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

notification/rabbitmq_publish.py

ANSIBLE VERSION
ansible 2.7.0.dev0 (rabbitmq_basic_publish d1167887f5) last updated 2018/08/28 00:20:25 (GMT +1100)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/johni/.ansible/plugins/modules', u'/usr/share/ansible/plugins/modules']
  ansible python module location = /home/johni/ansible/hacking/ansible/lib/ansible
  executable location = /home/johni/ansible/hacking/ansible/bin/ansible
  python version = 2.7.12 (default, Dec  4 2017, 14:50:18) [GCC 5.4.0 20160609]
ADDITIONAL INFORMATION
---
- hosts: localhost
  gather_facts: no

  tasks:
    - rabbitmq_publish:
        url: "amqp://guest:guest@192.168.0.32:5672/%2F"
        queue: 'hello'
        body: "Hello world from ansible module rabitmq_publish"
        content_type: "text/plain"
      register: output
      delegate_to: localhost

    - debug: 
        var: output

    - rabbitmq_publish:
        url: "amqp://guest:guest@192.168.0.32:5672/%2F"
        queue: 'hello'
        src: 'ajax-loader.gif'
      register: output2
      delegate_to: localhost

    - debug: 
        var: output2

Output:

$ ansible-playbook rabbit_test.yml

PLAY [localhost] *******************************************************************************************************************************************************************************************

TASK [rabbitmq_publish] ******************************************************************************************************************************************************************************
changed: [localhost -> localhost]

TASK [debug] ***********************************************************************************************************************************************************************************************
ok: [localhost] => {
    "output": {
        "changed": true, 
        "failed": false, 
        "msg": "Hello world from ansible module rabitmq_publish"
    }
}
......

@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_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. test This PR relates to tests. labels Aug 27, 2018
@ansibot
Copy link
Contributor

ansibot commented Aug 27, 2018

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

lib/ansible/modules/notification/rabbitmq_basic_publish.py:118:7: singleton-comparison Comparison to None should be 'expr is None'
lib/ansible/modules/notification/rabbitmq_basic_publish.py:121:7: singleton-comparison Comparison to None should be 'expr is None'

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

lib/ansible/modules/notification/rabbitmq_basic_publish.py:118:17: E711 comparison to None should be 'if cond is None:'
lib/ansible/modules/notification/rabbitmq_basic_publish.py:121:20: E711 comparison to None should be 'if cond is None:'
lib/ansible/modules/notification/rabbitmq_basic_publish.py:147:26: E128 continuation line under-indented for visual indent
lib/ansible/modules/notification/rabbitmq_basic_publish.py:148:26: E128 continuation line under-indented for visual indent
lib/ansible/modules/notification/rabbitmq_basic_publish.py:149:26: E128 continuation line under-indented for visual indent

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

lib/ansible/modules/notification/rabbitmq_basic_publish.py:0:0: E312 No RETURN provided
lib/ansible/modules/notification/rabbitmq_basic_publish.py:0:0: E325 argument_spec for "auto_delete" defines type="bool" but documentation does not
lib/ansible/modules/notification/rabbitmq_basic_publish.py:0:0: E325 argument_spec for "durable" defines type="bool" but documentation does not
lib/ansible/modules/notification/rabbitmq_basic_publish.py:0:0: E325 argument_spec for "exclusive" defines type="bool" but documentation does not

click here for bot help

@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 27, 2018
@ryansb ryansb removed the needs_triage Needs a first human triage before being processed. label Aug 28, 2018
@Im0 Im0 changed the title RabbitMQ basic publisher WIP: RabbitMQ basic publisher Aug 30, 2018
@Im0 Im0 mentioned this pull request Aug 30, 2018
@ansibot ansibot added the WIP This issue/PR is a work in progress. Nevertheless it was shared for getting input from peers. label Aug 30, 2018
@ansibot ansibot added the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Sep 7, 2018
@ansibot ansibot removed ci_verified Changes made in this PR are causing tests to fail. stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Oct 1, 2018
@ansibot
Copy link
Contributor

ansibot commented Oct 1, 2018

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

lib/ansible/module_utils/rabbitmq.py:76:79: undefined-variable Undefined variable 'queue'
lib/ansible/module_utils/rabbitmq.py:92:30: undefined-variable Undefined variable 'AnsibleError'
lib/ansible/module_utils/rabbitmq.py:97:61: undefined-variable Undefined variable 'count'

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

lib/ansible/modules/notification/rabbitmq_basic_publish.py:0:0: E312 No RETURN provided
lib/ansible/modules/notification/rabbitmq_basic_publish.py:0:0: E317 "queue" is marked as required but specifies a default. Arguments with a default should not be marked as required
lib/ansible/modules/notification/rabbitmq_basic_publish.py:0:0: E325 argument_spec for "auto_delete" defines type="bool" but documentation does not
lib/ansible/modules/notification/rabbitmq_basic_publish.py:0:0: E325 argument_spec for "durable" defines type="bool" but documentation does not
lib/ansible/modules/notification/rabbitmq_basic_publish.py:0:0: E325 argument_spec for "exclusive" defines type="bool" but documentation does not

click here for bot help

@Im0
Copy link
Contributor Author

Im0 commented Oct 2, 2018

Considering adding a "contents" list of dictionaries to rabbitmq_basic_publish to allow posting multiple items in one task. If specified, would need to account for different queues/routing for each message. (Won't do... user can loop over using with_items)

Look at removing 'required' for queue name. Pika/Rabbit should return a random queue name.

Look at adding a blocking subscriber to retrieve messages.

Fix up new argument checks. Eg. host/port/user/pass/vhost, need to check if they are all supplied if url is not supplied.

Consider how to add filename for binary uploads. (Added a filename header)

@ansibot
Copy link
Contributor

ansibot commented Oct 2, 2018

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

lib/ansible/modules/notification/rabbitmq_basic_publish.py:40:33: W291 trailing whitespace

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

lib/ansible/modules/notification/rabbitmq_basic_publish.py:0:0: E312 No RETURN provided
lib/ansible/modules/notification/rabbitmq_basic_publish.py:0:0: E317 "queue" is marked as required but specifies a default. Arguments with a default should not be marked as required

click here for bot help

@ansibot
Copy link
Contributor

ansibot commented Oct 8, 2018

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

lib/ansible/modules/notification/rabbitmq_basic_publish.py:0:0: E312 No RETURN provided

click here for bot help

@ansibot ansibot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Oct 10, 2018
@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. and removed stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels Oct 16, 2018
@Im0
Copy link
Contributor Author

Im0 commented Oct 17, 2018

@Jorge-Rodriguez fyi re modifications to module_utils/rabbitmq.py.

@ansibot
Copy link
Contributor

ansibot commented Nov 3, 2018

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

lib/ansible/modules/notification/rabbitmq_publish.py:0:0: E326 Value for "choices" from the argument_spec ([]) for "proto" does not match the documentation (['ampqs', 'ampq'])

click here for bot help

@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 Nov 3, 2018
@tonyseek
Copy link
Contributor

tonyseek commented Nov 4, 2018

May we call it amqp_publish instead of rabbitmq_publish? The RabbitMQ is not the only implementation of AMQP broker.

@Im0
Copy link
Contributor Author

Im0 commented Nov 4, 2018

@tonyseek Thanks for the comment.

I'm not against the idea of changing the name. Just wondered, have you tested the module against another broker? Does it work fine? I've only tested against Rabbit, including all the integration test plans.

@ansibot ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Nov 4, 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 Nov 4, 2018
@Im0 Im0 changed the title RabbitMQ publisher module AMQP publisher module (was RabbitMQ publisher module) Nov 5, 2018
@dch
Copy link

dch commented Nov 5, 2018

@tonyseek given that pika is only an AMQP 0.9.1 client and that the current AMQP standard is 1.0 which most other brokers provide as either default or only option, I suspect this will cause confusion.

e.g. azure has AMQP (1.0), as does apache qpid, but their clients and services don't support AMQP 0.9.1.

https://www.rabbitmq.com/specification.html is a good explanation of the differences.

Unless somebody's actually confirmed this works, with a popular and widely deployed AMQP 0.9.1 compatible alternative broker, I'd stick firmly to calling it rabbitmq until demonstrated otherwise, or perhaps the middle ground amqp091 or something like it to allow a future amqp10 module in the same namespace.

@Im0
Copy link
Contributor Author

Im0 commented Nov 6, 2018

@dch @tonyseek - I was considering leaving the module name as rabbitmq_publish but call the module_utils file amqp.py and the class something like AMQP091Client... this way module_utils\amqp.py will have a space an AMQP10 implementation. Thoughts?

@tonyseek
Copy link
Contributor

tonyseek commented Nov 6, 2018

@dch Yes. They (AMQP 0.9 and AMQP 1.0) are different protocols. I think amqp09 is a well enough name personally.

@Im0 Maybe we don't need to worry about AMQP10 because it is another protocol. I think amqp09_publish and rabbitmq_publish are both nice names. I don't prefer one than another one.

I propose to consider using protocol name because I saw the letsencrypt_account module was renamed to acme_account. I am not sure whether it is a custom in ansible community to use spec name instead of implementation name.

@Im0 Im0 changed the title AMQP publisher module (was RabbitMQ publisher module) RabbitMQ publisher module Nov 8, 2018
@Im0
Copy link
Contributor Author

Im0 commented Nov 8, 2018

@pilou- @dch @mattclay - thanks for the reviews and feedback.
@pilou- - thanks to you especially for your (as usual) great constructive feedback. I think I covered your feedback and resolved them. If I've missed anything please let me know (when you have time).

Otherwise, I'm hoping you can give your support to shipit. Thanks.

@pilou-
Copy link
Contributor

pilou- commented Nov 14, 2018

LGTM (I am not an RabbitMQ user).

@dch
Copy link

dch commented Nov 14, 2018

image

👏 nice work @Im0

@willthames willthames merged commit 54c54fc into ansible:devel Nov 15, 2018
@willthames
Copy link
Contributor

Merged, thanks @Im0 !

mjmayer pushed a commit to mjmayer/ansible that referenced this pull request Nov 30, 2018
* RabbitMQ basic publisher

* Split out of a module_util. Preparing for binary posts.

* Can now send a file to the queue.

* Allowing an empty queue to be used so RabbitMQ returns a random queue.

* Added RETURN docstring.

* Updated and added tests.  Now returns a dictionary with msg, content_type and queue published to.

* Extra tests and introduced a none url method of providing server host details.

* Added testing and errors for url/host parameters.

* Updating RETURN sample

* Added an image file for testing binary publishing.

* Minor changes to test.

* Added filename key/value to headers if a binary file is published.

* Adding ability to specify headers.

* Renaming to rabbitmq_publish

* Changed tests to reflect name, and, preparing for testing headers.

* Updated some documentation

* Minor pip install update

* Modifications after feedback.

* Updates based on feedback.

* Fixing pep8 issue.

* Updating module and module_util name to amqp.

* Reverting back to rabbitmq_publish naming.

* Minor addition to notes.
Tomorrow9 pushed a commit to Tomorrow9/ansible that referenced this pull request Dec 4, 2018
* RabbitMQ basic publisher

* Split out of a module_util. Preparing for binary posts.

* Can now send a file to the queue.

* Allowing an empty queue to be used so RabbitMQ returns a random queue.

* Added RETURN docstring.

* Updated and added tests.  Now returns a dictionary with msg, content_type and queue published to.

* Extra tests and introduced a none url method of providing server host details.

* Added testing and errors for url/host parameters.

* Updating RETURN sample

* Added an image file for testing binary publishing.

* Minor changes to test.

* Added filename key/value to headers if a binary file is published.

* Adding ability to specify headers.

* Renaming to rabbitmq_publish

* Changed tests to reflect name, and, preparing for testing headers.

* Updated some documentation

* Minor pip install update

* Modifications after feedback.

* Updates based on feedback.

* Fixing pep8 issue.

* Updating module and module_util name to amqp.

* Reverting back to rabbitmq_publish naming.

* Minor addition to notes.
@dagwieers dagwieers added the rabbitmq RabbitMQ community label Jan 28, 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 community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. new_module This PR includes a new module. new_plugin This PR includes a new plugin. rabbitmq RabbitMQ community support:community This issue/PR relates to code supported by the Ansible community. test This PR relates to tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants