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

Zabbix: new module zabbix_user #56815

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@sky-joker
Copy link
Contributor

commented May 22, 2019

SUMMARY

This pull request adds a module to manage Zabbix users.
Tested with Zabbix Server(3.0, 4.0, 4.2).

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

zabbix_user

ADDITIONAL INFORMATION

I think it would be useful to manage Zabbix users with Ansible.
For LDAP authentication with Zabbix, user registration is required for Zabbix and LDAP.
For example, by combining this module and the LDAP module, user registration can be performed at one time.

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

@Akint @D3DeFi @K-DOT @RedWhiteMiko @abulimov @akomic @cove @dj-wasabi @eikef @harrisongu @logan2211 @rubentsirunyan @sookido

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented May 22, 2019

sky-joker added some commits May 23, 2019

[update] zabbix_user module
* Modified to not use `()` in `if`
* Added `absent` check
@D3DeFi
Copy link
Contributor

left a comment

Thank you for this contribution, it is very well written and I like it :) Except a few minor cosmetic issues, there is problem that module is not idempotent (result is always changed=true) even if no options were changed.

Do you have time to address this idempotency problem please? If not, I can vote for this module to be merged and open an issue that we (you or me) can address later.


DOCUMENTATION = '''
module: zabbix_user
short_description: Zabbix user creates/updates/deletes

This comment has been minimized.

Copy link
@D3DeFi

D3DeFi May 24, 2019

Contributor
Suggested change
short_description: Zabbix user creates/updates/deletes
short_description: Create/update/delete Zabbix users

Just to be compatible with other ZBX modules

This comment has been minimized.

Copy link
@sky-joker

sky-joker May 26, 2019

Author Contributor

Fixed the pointed out line.

'''

try:
from zabbix_api import ZabbixAPI, ZabbixAPISubClass

This comment has been minimized.

Copy link
@D3DeFi

D3DeFi May 24, 2019

Contributor
Suggested change
from zabbix_api import ZabbixAPI, ZabbixAPISubClass
from zabbix_api import ZabbixAPI

unused import

This comment has been minimized.

Copy link
@sky-joker

sky-joker May 26, 2019

Author Contributor

Fixed the pointed out line.

else:
module.exit_json(changed=False)

module.exit_json(changed=True, user_ids=user_ids)

This comment has been minimized.

Copy link
@D3DeFi

D3DeFi May 24, 2019

Contributor

result is always changed when rerunning playbook with your first example (create of zabbix user). This breaks module idempotency. I believe this can be solved by comparing all update_user() arguments with information returned from user.check_user_exist(alias)

This comment has been minimized.

Copy link
@sky-joker

sky-joker May 24, 2019

Author Contributor

It's a nice idea!
I will try to that advice.

This comment has been minimized.

Copy link
@sky-joker

sky-joker May 26, 2019

Author Contributor

Added user_parameter_difference_check method to compare user parameter.

@sky-joker

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

@D3DeFi

Thank you for the review and advices!

Do you have time to address this idempotency problem please?

Well noted with thanks.
I will deal with this problem.

[update] Made multiple fixes to zabbix_user module.
Fixed the following adviced.
- #56815 (comment)
- #56815 (comment)
- #56815 (comment)

Added `override_passwd` option because new password cannot be compared with old password.
In the case of zabbix 3.2 or less, it was necessary to use the updatemedia method to update the media, therefore it was changed to use.

Tested with Zabbix Server(3.0, 3.2, 3.4, 4.0, 4.2).
@D3DeFi

D3DeFi approved these changes May 26, 2019

@D3DeFi

This comment has been minimized.

Copy link
Contributor

commented May 26, 2019

shipit

@ansibot ansibot added the stale_ci label Jun 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.