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 MQTT notification callback plugin #23769
Conversation
c3cd607
to
9d0ec4b
Compare
lib/ansible/plugins/callback/mqtt.py
Outdated
out_topic = self.base_topic + '/' + topic | ||
publish.single(out_topic, msg, hostname=self.mqtt_hostname, | ||
port=self.port, auth=self.auth, tls=self.tls, | ||
client_id=self.client_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think adding a timestamp field would be nice.
CALLBACK_NAME = 'mqtt' | ||
CALLBACK_NEEDS_WHITELIST = True | ||
|
||
def _parse_config(self, config): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a thought, you could leverage get_config from constants.py to allow end users to use env variables or ansible.cfg as they prefer instead of relying on a different config file.
Example usage in ARA: https://github.com/openstack/ara/blob/master/ara/config.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 on get_config for clarity.
Pinged you on IRC about it but for posterity... When dealing with a task that has items (ex: with_items), the result._result var of the task is actually pretty empty -- it doesn't contain the results of the items, just a summary message like "All items completed". The results are kept in the items' result variables. Maybe you want to pick those up, I don't know. Note that I just filed a bug about the inconsistent callback execution with items and task completion: #24207 |
@dmsimard The latest revision fixes mots of your comments. I still have to investigate the config switch, I haven't had a chance to dig into that yet though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits, but I think there's one actual problem to address.
|
||
Optionally these values can be set via a yaml file in /etc/mqtt_client.yaml, | ||
or $HOME/.mqtt_client.yaml (where each env variable lowercase is a key in | ||
the yaml files) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like some things could be in ENV, some could be in the user's dot file, and some could be in /etc/ and as long as they aren't overlapping, it'll gather all the parts from all the files. That's fine, but might want to mention it in the docs (could be as a follow up after the fact too). Additionally the ordering should be documented as well (ENV, user dir, /etc/ dir)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, I say below that ENV takes first priority, but I should outline the config file priority too. I've got to respin this anyway, I'll add that bit on in the next rev
def _publish(self, topic, msg): | ||
out_topic = self.base_topic + '/' + topic | ||
msg['timestamp'] = datetime.datetime.utcnow().isoformat() | ||
publish.single(out_topic, json.dumps(msg), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this ever raise an error? Should it be trapped and logged correctly, or is there infra around callbacks that takes care of this already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's true, I don't know if there is any infra around callback plugins to do that or not. (I'd have to dig through the call path to find out, most of the interface isn't documented). It probably doesn't hurt to collect the errors either way though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, looking at it a bit more closely I'm not sure how to setup logging for something like this from the plugin context, I couldn't find any good examples of that to base it off of. If you have any suggestions I'll add that on in a future patch.
lib/ansible/plugins/callback/mqtt.py
Outdated
'status': "FAILED", | ||
'host': self.hostname, | ||
'ansible_host': host, | ||
'playbook_id': self.session, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above playbook_id
was set to self.uuid
, but here it's being set to self.session
which above is used for session
. Maybe some copy/pasta got messed up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, yep that's a good catch. I'll respin this now
c0031e9
to
89b72b7
Compare
The latest revision addresses the review comments from @j2sol |
The test
|
This commit adds a new callback for emitting mqtt notifications for tasks and playbooks.
89b72b7
to
8d7b239
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good know to me. If I get info on handling tracebacks in a callback I'll respond.
shipit |
shipit |
MQTT_KEYFILE (optional): The path pointing to the PEM encoded client | ||
private key | ||
MQTT_CLIENT_ID (optional): MQTT client identifier, defaults to | ||
hostname + pid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This callback needs to be updated to utilize "config". See other current callback plugins for their DOCUMENTATION
and set_options
method.
@bcoca can you give us a hand with that? |
Please submit new callback plugin requests to https://github.com/ansible-collections/community.general. Thanks for your contribution. |
This commit adds a new callback for emitting mqtt notifications for
tasks and playbooks.
SUMMARY
This commit adds a new callback module for emitting mqtt notifications for
tasks and playbooks.
ISSUE TYPE
COMPONENT NAME
ANSIBLE VERSION
ADDITIONAL INFORMATION