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 Zabbix inventory support #45458

Merged
merged 9 commits into from
Feb 21, 2018
Merged

Conversation

mchugh19
Copy link
Contributor

What does this PR do?

Add Zabbix inventory handling to zabbix execution module
Support Zabbix inventory handling in zabbix_host state

Tests written?

No

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

Christian McHugh added 2 commits January 16, 2018 07:54
Copy link
Contributor

@rallytime rallytime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @mchugh19!

I have a couple of small comments.

Can you also write some tests for these new functions?

Retrieve host inventory according to the given parameters.
See: https://www.zabbix.com/documentation/2.4/manual/api/reference/host/object#host_inventory

.. versionadded:: Oxygen
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be updated to Fluorine as we've completed the feature freeze for oxygen already.

NOTE: This function accepts all standard host: keyword argument names for inventory
see: https://www.zabbix.com/documentation/2.4/manual/api/reference/host/object#host_inventory

.. versionadded:: Oxygen
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here.

@@ -1058,6 +1058,97 @@ def host_update(hostid, **connection_args):
return ret


def hostinventory_get(hostids, **connection_args):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think host_inventory_get might be a better name here. I think it's easier to read.

return ret


def hostinventory_set(hostid, **connection_args):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

host_inventory_set on this function is easier to read, IMHO, as well.

@rallytime rallytime added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Jan 22, 2018
Rename module functions
Cleanup minor lint errors
@mchugh19
Copy link
Contributor Author

I'm having a bit of trouble coming up with the mocking necessary for the _query method. Could anyone guide me, and I'll write up the test cases?

@mchugh19
Copy link
Contributor Author

mchugh19 commented Feb 4, 2018

Took another crack at it and got unit tests going. Added tests for the existing user_* and the newly added host_inventory_* methods.

@rallytime rallytime removed the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label Feb 8, 2018
@rallytime
Copy link
Contributor

@mchugh19
Copy link
Contributor Author

mchugh19 commented Feb 8, 2018

Lint output cleaned up. I had an issue running pylint from python2, but just corrected that.

@mchugh19
Copy link
Contributor Author

mchugh19 commented Feb 8, 2018

The referenced test failures seem unrelated as they had to do with other testing scripts like vmware.

@mchugh19
Copy link
Contributor Author

I think all requested updates have been made. Is there anything still outstanding?

@rallytime
Copy link
Contributor

@mchugh19 This looks good to me but the tests timed out before I could double check them. I'll keep an eye on this so we can get it in. Thanks!

@rallytime rallytime merged commit fd0482b into saltstack:develop Feb 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants