-
Notifications
You must be signed in to change notification settings - Fork 158
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
Added method to ask for active checks #72
base: master
Are you sure you want to change the base?
Conversation
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.
Sorry for late review. I think it's good idea to support active checks. But I think it require some changes to follow best practices for easy maintaining and backward compatibility.
:rtype: str | ||
:return: Formatted zabbix request | ||
""" | ||
request = '{"request":"active checks","host":"%s"}' % host |
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.
It's better use format
method here.
But actually, I don't think we need this as separate function, because it's just 2-3 lines of code and it used just in one place. I think it should go to send_active_checks
logger.debug('Request: %s', request) | ||
return request | ||
|
||
def send_active_checks(self, host): |
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 function returns list of active checks from zabbix server, then why it calls send_active_checks
instead of, an example, get_active_checks
?
request = self._create_active_checks_request(host) | ||
packet = self._create_packet(request) | ||
|
||
for host_addr in self.zabbix_uri: |
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 this part must be refactored as a function _send
because you duplicating big chunk code of _chunk_send
here.
if self.checks is None: | ||
return '' | ||
else: | ||
return "[%s]" % ", ".join([ch.__repr__() for ch in self.checks]) |
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.
It's better represent it as json. Which you doing in ZabbixCheck.__repr__
. I think it should be consisted.
# lastlogsize is optional in 2.2; required in 2.4, 3.0, 3.2, 3.4 | ||
# mtime is unused in 3.2; required in 2.4, 3.0, 3.2, 3.4 | ||
ch = ZabbixCheck( | ||
o.get('key'), |
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.
If some keys are required and not optional, we should get them explicitly to raise exception on error. Otherwise we may get empty checks on error.
self.key = str(key) | ||
self.delay = delay | ||
self.lastlogsize = lastlogsize | ||
self.mtime = mtime |
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.
It's usefull decorate properties as actually properties.
Example:
@property
def chunk(self):
return self._chunk
Asks the Zabbix server for the active checks that are needed for a given host, following the protocol documented here: https://www.zabbix.com/documentation/3.4/manual/appendix/items/activepassive?s[]=agent&s[]=protocol
This PR includes a few unit tests for the new methods. I also wrote a functional test (not included here), but I wasn't sure how to update the Zabbix test data to make it pass. If you don't mind getting the test to pass yourself, let me know and I'll add it as a separate commit.
Thanks for this project!