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

New zabbix_user_group module #58051

Open
wants to merge 3 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@emriver
Copy link

commented Jun 19, 2019

SUMMARY

New module to manage Zabbix user groups.
Tested on Zabbix 4.0.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

zabbix_user_group

ADDITIONAL INFORMATION

@emriver emriver changed the title new zabbix_user_group module (#4) New zabbix_user_group module Jun 19, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2019

The test ansible-test sanity --test pep8 [explain] failed with 19 errors:

lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:140:52: E231 missing whitespace after ':'
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:140:60: E231 missing whitespace after ':'
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:151:52: E231 missing whitespace after ':'
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:151:74: E231 missing whitespace after ':'
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:151:99: E231 missing whitespace after ':'
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:151:116: E231 missing whitespace after ':'
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:180:36: E231 missing whitespace after ':'
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:180:59: E231 missing whitespace after ':'
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:197:9: E265 block comment should start with '# '
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:209:9: E265 block comment should start with '# '
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:210:9: E265 block comment should start with '# '
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:215:9: E265 block comment should start with '# '
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:249:1: E302 expected 2 blank lines, found 1
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:306:5: E265 block comment should start with '# '
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:313:5: E265 block comment should start with '# '
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:317:5: E265 block comment should start with '# '
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:330:9: E265 block comment should start with '# '
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:334:9: E265 block comment should start with '# '
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:339:1: E305 expected 2 blank lines after class or function definition, found 1

The test ansible-test sanity --test validate-modules [explain] failed with 12 errors:

lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:0:0: E305 DOCUMENTATION.options.users_status.defaut: extra keys not allowed @ data['options']['users_status']['defaut']. Got 'enabled'
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:0:0: E322 Argument 'state' is listed in the argument_spec, but not documented in the module documentation
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:0:0: E324 Argument 'state' in argument_spec defines default as ('present') but documentation defines default as (None)
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:0:0: E324 Argument 'users_status' in argument_spec defines default as ('enabled') but documentation defines default as (None)
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:0:0: E326 Argument 'state' in argument_spec defines choices as (['present', 'absent', 'dump']) but documentation defines choices as ([])
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:0:0: E327 Argument 'debug_mode' in documentation defines default as ('disabled') but this is incompatible with parameter type 'bool'
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:0:0: E337 Argument 'name' in argument_spec defines type as 'str' but documentation doesn't define type
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:0:0: E337 Argument 'rights' in argument_spec defines type as 'list' but documentation doesn't define type
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:0:0: E337 Argument 'users' in argument_spec defines type as 'list' but documentation doesn't define type
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:0:0: E338 Argument 'gui_access' in argument_spec uses default type ('str') but documentation doesn't define type
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:0:0: E338 Argument 'state' in argument_spec uses default type ('str') but documentation doesn't define type
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:0:0: E338 Argument 'users_status' in argument_spec uses default type ('str') but documentation doesn't define type

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:87:1: W293 blank line contains whitespace
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:335:5: E303 too many blank lines (2)
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:368:1: E305 expected 2 blank lines after class or function definition, found 1

click here for bot help

@ansibot ansibot added the ci_verified label Jun 25, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:87:1: W293 blank line contains whitespace

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 2019

@emriver this PR contains the following merge commits:

Please rebase your branch to remove these commits.

click here for bot help

new zabbix_user_group module (#4)
* new zabbix_user_group module

* Fix typo

* fix githubid

@emriver emriver force-pushed the ovh:new_zabbix_user_group_module branch from 2d2bb53 to 454c01f Jun 25, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 25, 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 Jun 26, 2019

The test ansible-test sanity --test pylint [explain] failed with 3 errors:

lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:312:26: trailing-whitespace Trailing whitespace
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:313:29: undefined-variable Undefined variable 'missing_required_lib'
lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:313:119: undefined-variable Undefined variable 'ZBX_IMP_ERR'

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/monitoring/zabbix/zabbix_user_group.py:312:27: W291 trailing whitespace

click here for bot help

@ansibot ansibot added the ci_verified label Jun 26, 2019

@emriver emriver force-pushed the ovh:new_zabbix_user_group_module branch from 709b396 to 2dc4b51 Jun 26, 2019

@ansibot ansibot removed the ci_verified label Jun 26, 2019

@D3DeFi
Copy link
Contributor

left a comment

Hello and thank you for the contribution @emriver ! I've tested this module with almost every currently supported zabbix version and it seems to be working properly, except a small inconsistency with identifying users (see my comments in the review).

After required adjustments are made, I think this module will be ready to be merged.

- Name of the Zabbix user group
required: true
type: str
debug_mode:

This comment has been minimized.

Copy link
@D3DeFi

D3DeFi Jun 27, 2019

Contributor

If I am reading it correctly, debug_mode is a specific thing for a whole Zabbix API and not just for this module.

This is not something we use in other zabbix modules and its usefulness for lets say an automated playbook is questionable. From the point of user's view, I shouldn't be expected to debug anything if only thing I am interested in is to create a few user groups.

Can you provide some reasoning behind this please? I can understand that it is a good help for development purposes.

This comment has been minimized.

Copy link
@emriver

emriver Jun 27, 2019

Author

I think there is a misunderstanding with this property.
From my understanding the debug_mode, (described here) allows Zabbix users to click on a Debug button to troubleshoot front end web pages.

This comment has been minimized.

Copy link
@D3DeFi

D3DeFi Jun 28, 2019

Contributor

Ok, my bad. Thanks for clarification! :)


userids = []
for user in users:
user_array = self._zapi.user.get({'filter': {'name': user}})

This comment has been minimized.

Copy link
@D3DeFi

D3DeFi Jun 27, 2019

Contributor

Using name as an identifier for users can lead to a lots of confusion. Imagine that I have three users named John but with different aliases.

Also this makes users without name attribute (service users or guest account) "invisible" for this module. Please, rework this to use alias instead as Zabbix API documentation states that it is the required attribute for user object.

This comment has been minimized.

Copy link
@emriver

emriver Jun 27, 2019

Author

Good catch! I will update the code as requested


userids = []
for user in users:
user_array = self._zapi.user.get({'filter': {'name': user}})

This comment has been minimized.

Copy link
@D3DeFi

D3DeFi Jun 27, 2019

Contributor

Using name as an identifier for users can lead to a lots of confusion. Imagine that I have three users named John but with different aliases.

Also this makes users without name attribute (service users or guest account) "invisible" for this module. Please, rework this to use alias instead as Zabbix API documentation states that it is the required attribute for user object.

@D3DeFi

D3DeFi approved these changes Jun 28, 2019

Copy link
Contributor

left a comment

LGTM

@rubentsirunyan
Copy link
Contributor

left a comment

shipit

@D3DeFi

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

Thanks for your work @emriver . This should now get merged soon, by someone with merge permissions (new modules are not auto-merged by default if I am not mistaken)

@rubentsirunyan thanks for the review

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.