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

new modules: zabbix_host, zabbix_group, zabbix_screen, zabbix_webcheck #6034

Closed
wants to merge 12 commits into
base: devel
from

Conversation

Projects
None yet
9 participants
@cove
Contributor

cove commented Feb 16, 2014

These modules allow you to add/update hosts, put them in the correct groups and link in the monitoring templates, the basic requirements for bring a new host online. Been in production use since late last year, appear to be working fine against Zabbix 2.0.

@abulimov

This comment has been minimized.

Show comment
Hide comment
@abulimov

abulimov Feb 25, 2014

Contributor

Looks great. Why didn't you used any existing Zabbix API library? Maintaining own api realization coud become a problem in the future... And i think, it would be better to have a unified parameter names in your modules and my zabbix_maintenance module (#5062). For example, 'zabbix_login_user' in my module is called just 'user', maybe it would be better to call it 'login_user' in all zabbix modules?

Contributor

abulimov commented Feb 25, 2014

Looks great. Why didn't you used any existing Zabbix API library? Maintaining own api realization coud become a problem in the future... And i think, it would be better to have a unified parameter names in your modules and my zabbix_maintenance module (#5062). For example, 'zabbix_login_user' in my module is called just 'user', maybe it would be better to call it 'login_user' in all zabbix modules?

@cove

This comment has been minimized.

Show comment
Hide comment
@cove

cove Mar 8, 2014

Contributor

There were a few other issues in our env which is why the lib was in the module, however it looks like we can switch over to using the regular lib, and possibly submit a patch to for the lib instead. Fully agree on the login_user, yep.

On a side note, we also have a module for creating screens at this point, so we'll be submitting that as well, we're a bit busy right now, but should be in before the end of the month if not sooner.

Contributor

cove commented Mar 8, 2014

There were a few other issues in our env which is why the lib was in the module, however it looks like we can switch over to using the regular lib, and possibly submit a patch to for the lib instead. Fully agree on the login_user, yep.

On a side note, we also have a module for creating screens at this point, so we'll be submitting that as well, we're a bit busy right now, but should be in before the end of the month if not sooner.

@jimi-c

This comment has been minimized.

Show comment
Hide comment
@jimi-c

jimi-c Mar 11, 2014

Member

I think we're going to hold off on this, and let you guys work the common param names out.

@cove, if you do stick with the urllib method, you'll need to start using the new fetch_url code that is in lib/ansible/module_utils/urls.py (see some of the other new modules that use it for examples). The reason for this is that by default, urllib* doesn't validate SSL certs, so it's a bit unsafe to use directly. The new fetch_url() method handles cert validation, but also allows you to set a boolean to ignore validation for internal systems using self-signed certs.

Thanks!

Member

jimi-c commented Mar 11, 2014

I think we're going to hold off on this, and let you guys work the common param names out.

@cove, if you do stick with the urllib method, you'll need to start using the new fetch_url code that is in lib/ansible/module_utils/urls.py (see some of the other new modules that use it for examples). The reason for this is that by default, urllib* doesn't validate SSL certs, so it's a bit unsafe to use directly. The new fetch_url() method handles cert validation, but also allows you to set a boolean to ignore validation for internal systems using self-signed certs.

Thanks!

@abulimov

This comment has been minimized.

Show comment
Hide comment
@abulimov

abulimov Mar 14, 2014

Contributor

@cove, I've updated my zabbix_maintenance module to use unified parameter names. Please, take a look and feel free to comment if you don't want to use some particular parameter name.

Contributor

abulimov commented Mar 14, 2014

@cove, I've updated my zabbix_maintenance module to use unified parameter names. Please, take a look and feel free to comment if you don't want to use some particular parameter name.

@cove

This comment has been minimized.

Show comment
Hide comment
@cove

cove Mar 15, 2014

Contributor

@abulimov these look good to me, we'll update to match those.

Contributor

cove commented Mar 15, 2014

@abulimov these look good to me, we'll update to match those.

@mpdehaan mpdehaan added the P2 label Mar 19, 2014

@mpdehaan mpdehaan added the module label Mar 27, 2014

@cove

This comment has been minimized.

Show comment
Hide comment
@cove

cove Mar 28, 2014

Contributor

@abulimov can you try these updated modules out and see if they work for you?

Contributor

cove commented Mar 28, 2014

@abulimov can you try these updated modules out and see if they work for you?

@abulimov

View changes

Show outdated Hide outdated library/monitoring/zabbix_group
@abulimov

View changes

Show outdated Hide outdated library/monitoring/zabbix_host
@abulimov

View changes

Show outdated Hide outdated library/monitoring/zabbix_host
@srgvg

This comment has been minimized.

Show comment
Hide comment
@srgvg

srgvg Mar 28, 2014

Member

Bumped in the same problems. API has changed, Examples talk about "host_names" whilst the parameter is "host_name", ..

Also:

  • some parameters set as required in the documentation, but this is notr enforced in the AnsibleModule declaration.
  • No support for check mode
  • values for status: 0 and 1 should better be replaced by enabled/disabled
  • no support to remove/delete a host?
Member

srgvg commented Mar 28, 2014

Bumped in the same problems. API has changed, Examples talk about "host_names" whilst the parameter is "host_name", ..

Also:

  • some parameters set as required in the documentation, but this is notr enforced in the AnsibleModule declaration.
  • No support for check mode
  • values for status: 0 and 1 should better be replaced by enabled/disabled
  • no support to remove/delete a host?

@mpdehaan mpdehaan added P3 and removed P2 labels Mar 28, 2014

@cove

This comment has been minimized.

Show comment
Hide comment
@cove

cove Apr 3, 2014

Contributor

we have a few more fixes to submit here still.

Contributor

cove commented Apr 3, 2014

we have a few more fixes to submit here still.

@cove

This comment has been minimized.

Show comment
Hide comment
@cove

cove Apr 4, 2014

Contributor

@abulimov @sergevanginderachter let me know how these updates work for you. thx.

Contributor

cove commented Apr 4, 2014

@abulimov @sergevanginderachter let me know how these updates work for you. thx.

@abulimov

View changes

Show outdated Hide outdated library/monitoring/zabbix_host
@abulimov

This comment has been minimized.

Show comment
Hide comment
@abulimov

abulimov Apr 14, 2014

Contributor

I've tested zabbix_host, zabbix_group and zabbix_screen modules, and everything seemed fine except the one error i've mentioned above. But in zabbix_webcheck module i was unable to create application with error 'Failed to get the application example app id: list index out of range'. And error message was also 'list index out of range' when I tried to add a webcheck for non-existent host.

Contributor

abulimov commented Apr 14, 2014

I've tested zabbix_host, zabbix_group and zabbix_screen modules, and everything seemed fine except the one error i've mentioned above. But in zabbix_webcheck module i was unable to create application with error 'Failed to get the application example app id: list index out of range'. And error message was also 'list index out of range' when I tried to add a webcheck for non-existent host.

@mpdehaan mpdehaan added the P4 label Apr 17, 2014

@abulimov

This comment has been minimized.

Show comment
Hide comment
@abulimov

abulimov Apr 22, 2014

Contributor

@cove error in zabbix_host module that i've commented on line 263 is still not fixed. And i've commented another error in zabbix_webcheck module, line 239. Both fixes are trivial.

Contributor

abulimov commented Apr 22, 2014

@cove error in zabbix_host module that i've commented on line 263 is still not fixed. And i've commented another error in zabbix_webcheck module, line 239. Both fixes are trivial.

@abulimov

This comment has been minimized.

Show comment
Hide comment
@abulimov

abulimov Apr 24, 2014

Contributor

@cove error in webcheck module on line 239 is not fixed. No check for missing "steps" parameter, so module crashes when no "steps" specified.

Contributor

abulimov commented Apr 24, 2014

@cove error in webcheck module on line 239 is not fixed. No check for missing "steps" parameter, so module crashes when no "steps" specified.

@cove

This comment has been minimized.

Show comment
Hide comment
@cove

cove Apr 25, 2014

Contributor

@abulimov try now lmk

Contributor

cove commented Apr 25, 2014

@abulimov try now lmk

@abulimov

This comment has been minimized.

Show comment
Hide comment
@abulimov

abulimov Apr 25, 2014

Contributor

@cove check for missing "steps" works as expected, but I still have error when updating check:
on line 243

old_steps = list(web_check_obj['steps'].values())
AttributeError: 'list' object has no attribute 'values'

And when I simply remove .values() call, so this line looks like

old_steps = list(web_check_obj['steps'])

everything works fine. I'm testing this modules against Zabbix server 2.2.2, with Python 2.7.6.

Contributor

abulimov commented Apr 25, 2014

@cove check for missing "steps" works as expected, but I still have error when updating check:
on line 243

old_steps = list(web_check_obj['steps'].values())
AttributeError: 'list' object has no attribute 'values'

And when I simply remove .values() call, so this line looks like

old_steps = list(web_check_obj['steps'])

everything works fine. I'm testing this modules against Zabbix server 2.2.2, with Python 2.7.6.

@cove

This comment has been minimized.

Show comment
Hide comment
@cove

cove Apr 28, 2014

Contributor

@abulimov this appears to be due to a difference between 2.0,6 (the version we're on) and 2.2.2. We're looking into it.

Contributor

cove commented Apr 28, 2014

@abulimov this appears to be due to a difference between 2.0,6 (the version we're on) and 2.2.2. We're looking into it.

@cove

This comment has been minimized.

Show comment
Hide comment
@cove

cove Apr 30, 2014

Contributor

@abulimov let us know how this works.

Contributor

cove commented Apr 30, 2014

@abulimov let us know how this works.

@abulimov

This comment has been minimized.

Show comment
Hide comment
@abulimov

abulimov Apr 30, 2014

Contributor

@cove now everything works great, thanks! Hope it will soon be merged - you have made awesome work!

Contributor

abulimov commented Apr 30, 2014

@cove now everything works great, thanks! Hope it will soon be merged - you have made awesome work!

@cove

This comment has been minimized.

Show comment
Hide comment
@cove

cove Apr 30, 2014

Contributor

@abulimov ah great, thanks for all the testing! thanks goes to @harrisongu @TonyMinfeiDing and @Damonchenchengkong for writing the modules.

Contributor

cove commented Apr 30, 2014

@abulimov ah great, thanks for all the testing! thanks goes to @harrisongu @TonyMinfeiDing and @Damonchenchengkong for writing the modules.

@cove cove changed the title from new modules: zabbix_host and zabbix_group to new modules: zabbix_host, zabbix_group, zabbix_screen, zabbix_webcheck Apr 30, 2014

@srgvg

This comment has been minimized.

Show comment
Hide comment
@srgvg

srgvg May 5, 2014

Member

I tested the zabbix_host module, and AFAICS this works well, except it seems to always return a 'changed' state. looking at the code, this is confirmed as the return dict always gets changed=True.
So the feedback is never "idempotent".

Member

srgvg commented May 5, 2014

I tested the zabbix_host module, and AFAICS this works well, except it seems to always return a 'changed' state. looking at the code, this is confirmed as the return dict always gets changed=True.
So the feedback is never "idempotent".

@srgvg

View changes

Show outdated Hide outdated library/monitoring/zabbix_host
@srgvg

View changes

Show outdated Hide outdated library/monitoring/zabbix_host
@cove

This comment has been minimized.

Show comment
Hide comment
@cove

cove Jun 24, 2014

Contributor

@abulimov @sergevanginderachter can you validate @harrisongu's changes address your issues?

Contributor

cove commented Jun 24, 2014

@abulimov @sergevanginderachter can you validate @harrisongu's changes address your issues?

@srgvg

This comment has been minimized.

Show comment
Hide comment
@srgvg

srgvg Jun 24, 2014

Member

I re-imported those modules (_group and _host) and ran some tests, this seems to work well, without modifications.

Keep in mind I only tested some functionalities of some scripts, not everrything.

Member

srgvg commented Jun 24, 2014

I re-imported those modules (_group and _host) and ran some tests, this seems to work well, without modifications.

Keep in mind I only tested some functionalities of some scripts, not everrything.

@cove

This comment has been minimized.

Show comment
Hide comment
@cove

cove Jun 26, 2014

Contributor

@abulimov @sergevanginderachter ok, we've been using these scripts for quite a while at this point. Any objections to having it committed to the devel branch at this point?

Contributor

cove commented Jun 26, 2014

@abulimov @sergevanginderachter ok, we've been using these scripts for quite a while at this point. Any objections to having it committed to the devel branch at this point?

@jimi-c

This comment has been minimized.

Show comment
Hide comment
@jimi-c

jimi-c Jun 26, 2014

Member

@cove we'll try and get these into testing as soon as possible, however we do have quite a backlog at the moment, so having more +1's always helps.

Member

jimi-c commented Jun 26, 2014

@cove we'll try and get these into testing as soon as possible, however we do have quite a backlog at the moment, so having more +1's always helps.

@cove

This comment has been minimized.

Show comment
Hide comment
@cove

cove Sep 3, 2014

Contributor

@resmo Definitely send a PR, thanks! ;-)

Contributor

cove commented Sep 3, 2014

@resmo Definitely send a PR, thanks! ;-)

@mpdehaan

This comment has been minimized.

Show comment
Hide comment
@mpdehaan

mpdehaan Sep 29, 2014

Contributor

Hi!

Thanks very much for your interest in Ansible. It sincerely means a lot to us.

On September 26, 2014, due to enormous levels of contribution to the project Ansible decided to reorganize module repos, making it easier
for developers to work on the project and for us to more easily manage new contributions and tickets.

We split modules from the main project off into two repos, http://github.com/ansible/ansible-modules-core and http://github.com/ansible/ansible-modules-extras

If you still would like this pull request merged, we will need your help making this target the new repo. If you do not take any action, this
pull request unfortunately cannot be applied.

We apologize that we are not able to make this transition happen seamlessly, though this is a one-time change and your help is greatly appreciated --
this will greatly improve velocity going forward.

Both sets of modules will ship with Ansible, though they'll receive slightly different ticket handling.

To locate where a module lives between 'core' and 'extras'

Otherwise, if this is a new module:

It may be possible to re-patriate your pull requests automatically, one user-submitted approach for advanced git users
has been suggested at https://gist.github.com/willthames/afbaaab0c9681ed45619

Additionally, should you need more help with this, you can ask questions on:

Thank you very much!

Contributor

mpdehaan commented Sep 29, 2014

Hi!

Thanks very much for your interest in Ansible. It sincerely means a lot to us.

On September 26, 2014, due to enormous levels of contribution to the project Ansible decided to reorganize module repos, making it easier
for developers to work on the project and for us to more easily manage new contributions and tickets.

We split modules from the main project off into two repos, http://github.com/ansible/ansible-modules-core and http://github.com/ansible/ansible-modules-extras

If you still would like this pull request merged, we will need your help making this target the new repo. If you do not take any action, this
pull request unfortunately cannot be applied.

We apologize that we are not able to make this transition happen seamlessly, though this is a one-time change and your help is greatly appreciated --
this will greatly improve velocity going forward.

Both sets of modules will ship with Ansible, though they'll receive slightly different ticket handling.

To locate where a module lives between 'core' and 'extras'

Otherwise, if this is a new module:

It may be possible to re-patriate your pull requests automatically, one user-submitted approach for advanced git users
has been suggested at https://gist.github.com/willthames/afbaaab0c9681ed45619

Additionally, should you need more help with this, you can ask questions on:

Thank you very much!

@mpdehaan mpdehaan closed this Sep 29, 2014

@ansibot ansibot added feature and removed feature_pull_request labels Mar 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment