Improve Zabbix returner #61314

Flowdalic · 2021-12-02T12:43:58Z

Cleanup the Zabbix returner code and consider file Zabbix agent 2 configuration file also (see #61313).

Previously Zabbix returner would only consider zabbix_agentd.conf (Zabbix agent 1), now it also considers zabbix_agent2.conf (Zabbix agent 2) configuration files.

welcome · 2021-12-02T12:43:59Z

Hi there! Welcome to the Salt Community! Thank you for making your first contribution. We have a lengthy process for issues and PRs. Someone from the Core Team will follow up as soon as possible. In the meantime, here’s some information that may help as you continue your Salt journey.
Please be sure to review our Code of Conduct. Also, check out some of our community resources including:

There are lots of ways to get involved in our community. Every month, there are around a dozen opportunities to meet with other contributors and the Salt Core team and collaborate in real time. The best way to keep track is by subscribing to the Salt Community Events Calendar.
If you have additional questions, email us at [email protected]. We’re glad you’ve joined our community and look forward to doing awesome things with you!

OrangeDog · 2021-12-03T10:05:42Z

salt/returners/zabbix_return.py

-    else:
-        return False
+    if ZABBIX_SENDER is None or ZABBIX_CONFIG is None:
+        message = "No zabbix_sender and no zabbix_agent(d|2).conf found."


Would be clearer as if return elif return else return.

Consider also glob syntax for the message rather than regex syntax - i.e. {d,2}

Thanks for the feedback. I prefer the current approach. Is this a non-mandatory suggestion or a must? :)

Well I got confused reading it.
None of my suggestions are mandatory, but I strongly feel that a simpler control flow is better.

OrangeDog · 2021-12-03T10:06:34Z

salt/returners/zabbix_return.py



 def zabbix_send(key, output):
+    if not (ZABBIX_SENDER and ZABBIX_CONFIG):
+        raise AssertionError()


This isn't necessary, and should be an assert.

It's a cheap check where I usually opt for non-assert, but sure, can make it an assert.

OrangeDog · 2021-12-03T10:06:51Z

salt/returners/zabbix_return.py

    cmd = (
-        zbx()["sender"]
+        ZABBIX_SENDER


This may as well be an f-string now.

True, but I wanted to keep the diff as minimal as possible.

IIRC, an f-string wouldn't work here as the command needs to be an iterable of strings rather than a single string.

It is also possible to use the configuration file of Zabbix's Agent 2 for zabbix_sender, hence this file also also be considered as argument for zabbix_sender's --config command line paramter. Fixes saltstack#61313.

github-actions · 2022-09-21T16:40:42Z

Hi! I'm your friendly PR bot!

You might be wondering what I'm doing commenting here on your PR.

Yes, as a matter of fact, I am...

I'm just here to help us improve the documentation. I can't respond to
questions or anything, but what I can do, I do well!

Okay... so what do you do?

I detect modules that are missing docstrings or "CLI Example" on existing docstrings!
When I was created we had a lot of these. The documentation for these
modules need some love and attention to make Salt better for our users.

So what does that have to do with my PR?

I noticed that in this PR there are some files changed that have some of these
issues. So I'm leaving this comment to let you know your options.

Okay, what are they?

Well, my favorite, is that since you were making changes here I'm hoping that
you would be the most familiar with this module and be able to add some other
examples or fix any of the reported issues.

If I can, then what?

Well, you can either add them to this PR or add them to another PR. Either way is fine!

Well... what if I can't, or don't want to?

That's also fine! We appreciate all contributions to the Salt Project. If you
can't add those other examples, either because you're too busy, or unfamiliar,
or you just aren't interested, we still appreciate the contributions that
you've made already.

Whatever approach you decide to take, just drop a comment here letting us know!

Detected Issues (click me)

Check Known Missing Docstrings...........................................Failed
- hook id: invoke
- exit code: 1
/home/runner/.cache/pre-commit/repo80cx_edx/py_env-python3/lib/python3.9/site-packages/_distutils_hack/init.py:33: UserWarning: Setuptools is replacing distutils.

warnings.warn("Setuptools is replacing distutils.")

The function 'zbx' on 'salt/returners/zabbix_return.py' does not have a docstring

The function 'zabbix_send' on 'salt/returners/zabbix_return.py' does not have a docstring

The function 'returner' on 'salt/returners/zabbix_return.py' does not have a docstring

Found 3 errors

Thanks again!

github-actions · 2022-09-21T16:40:44Z

Hi! I'm your friendly PR bot!

You might be wondering what I'm doing commenting here on your PR.

Yes, as a matter of fact, I am...

I'm just here to help us improve the documentation. I can't respond to
questions or anything, but what I can do, I do well!

Okay... so what do you do?

I detect modules that are missing docstrings or "CLI Example" on existing docstrings!
When I was created we had a lot of these. The documentation for these
modules need some love and attention to make Salt better for our users.

So what does that have to do with my PR?

I noticed that in this PR there are some files changed that have some of these
issues. So I'm leaving this comment to let you know your options.

Okay, what are they?

Well, my favorite, is that since you were making changes here I'm hoping that
you would be the most familiar with this module and be able to add some other
examples or fix any of the reported issues.

If I can, then what?

Well, you can either add them to this PR or add them to another PR. Either way is fine!

Well... what if I can't, or don't want to?

That's also fine! We appreciate all contributions to the Salt Project. If you
can't add those other examples, either because you're too busy, or unfamiliar,
or you just aren't interested, we still appreciate the contributions that
you've made already.

Whatever approach you decide to take, just drop a comment here letting us know!

Detected Issues (click me)

Check Known Missing Docstrings...........................................Failed
- hook id: invoke
- exit code: 1
/home/runner/.cache/pre-commit/repo80cx_edx/py_env-python3/lib/python3.9/site-packages/_distutils_hack/init.py:33: UserWarning: Setuptools is replacing distutils.

warnings.warn("Setuptools is replacing distutils.")

The function 'zbx' on 'salt/returners/zabbix_return.py' does not have a docstring

The function 'zabbix_send' on 'salt/returners/zabbix_return.py' does not have a docstring

The function 'returner' on 'salt/returners/zabbix_return.py' does not have a docstring

Found 3 errors

Thanks again!

whytewolf · 2022-09-21T16:42:45Z

This is going to need tests. and pre-commit to be fixed.

MKLeb · 2022-10-07T05:09:49Z

Mentioning @waynew here since he is working on a returners test overhaul at the moment.

waynew

@Flowdalic thanks for the PR - this does need some tests - if you're up for adding them, and
If you need help - attend one of the Test Clinics on Twitch.

If you're not interested or able to add the tests here, we can close the PR and I can incorporate this work when I get to the zabbix returner tests as part of #61757.

If I don't hear back in the next ~week, I'll consider this PR as abandoned and go ahead and close it - if you want to pick it back up, just ping one of the core team on IRC or Slack and we can re-open it though!

waynew · 2022-10-10T17:49:42Z

salt/returners/zabbix_return.py



 def zabbix_send(key, output):
+    assert ZABBIX_SENDER and ZABBIX_CONFIG


suggestion assertions can be optimized away by Python. If it's essential then this should read:

if not (ZABBIX_SENDER or ZABBIX_CONFIG): raise SomeSaltError(...)

But that shouldn't be required, because __virtual__ will only return True if both of those are set, and Salt will refuse to load the module (or make available the module anyway, to be precise) if __virtual__ returns False.

waynew · 2022-10-10T17:51:30Z

salt/returners/zabbix_return.py

    cmd = (
-        zbx()["sender"]
+        ZABBIX_SENDER


IIRC, an f-string wouldn't work here as the command needs to be an iterable of strings rather than a single string.

waynew · 2022-10-19T15:14:31Z

Going ahead and closing this PR - @Flowdalic if you want to resume work on it, just reach out to a core team member in IRC or Slack (you can also try pinging on here, but GitHub notifications often just get lost in the noise 😓)

As mentioned in a previous comment, if you don't get this updated then I should end out getting to it with my work on #61757 👍 Thanks again!

Flowdalic requested a review from a team as a code owner December 2, 2021 12:43

Flowdalic requested review from krionbsd and removed request for a team December 2, 2021 12:43

Flowdalic force-pushed the zabbix-returner branch 2 times, most recently from 64791f2 to fc5112a Compare December 3, 2021 09:46

OrangeDog suggested changes Dec 3, 2021

View reviewed changes

Flowdalic added 2 commits December 3, 2021 11:21

zabbix_return: tidy up logic

5b08e38

zabbix_return: also consider zabbix_agent2.conf

34ab329

It is also possible to use the configuration file of Zabbix's Agent 2 for zabbix_sender, hence this file also also be considered as argument for zabbix_sender's --config command line paramter. Fixes saltstack#61313.

Flowdalic force-pushed the zabbix-returner branch from fc5112a to 34ab329 Compare December 3, 2021 10:21

Ch3LL added the P1 Priority 1 label Sep 19, 2022

Merge branch 'master' into zabbix-returner

b30c3e0

whytewolf added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Sep 21, 2022

MKLeb requested review from waynew and MKLeb and removed request for krionbsd October 7, 2022 05:09

waynew suggested changes Oct 10, 2022

View reviewed changes

MKLeb requested review from waynew and removed request for MKLeb October 10, 2022 18:06

waynew closed this Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Zabbix returner #61314

Improve Zabbix returner #61314

Flowdalic commented Dec 2, 2021 •

edited

Loading

welcome bot commented Dec 2, 2021

OrangeDog Dec 3, 2021

Flowdalic Dec 3, 2021

OrangeDog Dec 3, 2021

OrangeDog Dec 3, 2021

Flowdalic Dec 3, 2021

OrangeDog Dec 3, 2021

Flowdalic Dec 3, 2021

waynew Oct 10, 2022

github-actions bot commented Sep 21, 2022

github-actions bot commented Sep 21, 2022

whytewolf commented Sep 21, 2022

MKLeb commented Oct 7, 2022

waynew left a comment

waynew Oct 10, 2022

waynew Oct 10, 2022

waynew commented Oct 19, 2022



		def zabbix_send(key, output):
		assert ZABBIX_SENDER and ZABBIX_CONFIG

Improve Zabbix returner #61314

Improve Zabbix returner #61314

Conversation

Flowdalic commented Dec 2, 2021 • edited Loading

welcome bot commented Dec 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 21, 2022

Hi! I'm your friendly PR bot!

github-actions bot commented Sep 21, 2022

Hi! I'm your friendly PR bot!

whytewolf commented Sep 21, 2022

MKLeb commented Oct 7, 2022

waynew left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

waynew commented Oct 19, 2022

Flowdalic commented Dec 2, 2021 •

edited

Loading