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

Rabbitmq user permission fixes #49404

Merged
merged 6 commits into from Dec 7, 2018

Conversation

Projects
None yet
4 participants
@ptzianos
Contributor

ptzianos commented Dec 1, 2018

SUMMARY

Fixes #49120 but in a different way
Improves the situation with #29281

The purpose of this PR is to improve the idempotency of the module using Python features that behave consistently in version 2.7 and 3.5+.

Changes have been tested in Python 2.7.15, 3.5.6, 3.6.7, 3.7.1.

Changes:

  1. Further improves the method that checks whether or not the permissions of a user are modified to ensure consistent behavior.
  2. Adds some checks to ensure that the user permissions dictionary list is correct.
  3. Simplifies the code that modifies the permissions of the user.
  4. Includes tests for all the changes introduced in the PR plus some for existing functionality.
  5. Adds helper for simulating collections.Counter in Python 2.6

This is a first batch of fixes for this module. More will follow to eventually add full test coverage for the module and make the module idempotent.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

rabbitmq_user

@ansibot

This comment has been minimized.

Contributor

ansibot commented Dec 1, 2018

Hi @ptzianos, thank you for submitting this pull-request!

click here for bot help

@ansibot

This comment has been minimized.

Contributor

ansibot commented Dec 1, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Dec 1, 2018

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/messaging/rabbitmq/rabbitmq_user.py:120:0: ImportError: cannot import name Counter

click here for bot help

@ansibot ansibot added needs_revision and removed core_review labels Dec 1, 2018

@ptzianos

This comment has been minimized.

Contributor

ptzianos commented Dec 1, 2018

Fixing issues with Python 2.6 and 3.5

@ptzianos ptzianos force-pushed the ptzianos:rabbitmq_user_permission_fixes branch 3 times, most recently from 64581b8 to a0e7ac8 Dec 1, 2018

@ansibot

This comment has been minimized.

Contributor

ansibot commented Dec 1, 2018

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

lib/ansible/modules/messaging/rabbitmq/rabbitmq_user.py:122:0: ansible-bad-module-import Import external package or ansible.module_utils not ansible.utils.collections

The test ansible-test sanity --test import --python 2.6 [explain] failed with 1 error:

lib/ansible/modules/messaging/rabbitmq/rabbitmq_user.py:122:0: ImportError: No module named utils.collections

The test ansible-test sanity --test import --python 3.6 [explain] failed with 1 error:

lib/ansible/modules/messaging/rabbitmq/rabbitmq_user.py:122:0: ModuleNotFoundError: No module named 'ansible.utils'

The test ansible-test sanity --test import --python 3.5 [explain] failed with 1 error:

lib/ansible/modules/messaging/rabbitmq/rabbitmq_user.py:122:0: ImportError: No module named 'ansible.utils'

The test ansible-test sanity --test import --python 2.7 [explain] failed with 1 error:

lib/ansible/modules/messaging/rabbitmq/rabbitmq_user.py:122:0: ImportError: No module named utils.collections

The test ansible-test sanity --test import --python 3.7 [explain] failed with 1 error:

lib/ansible/modules/messaging/rabbitmq/rabbitmq_user.py:122:0: ModuleNotFoundError: No module named 'ansible.utils'

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

lib/ansible/utils/collections.py:0:0: should not have a shebang

click here for bot help

@ptzianos ptzianos force-pushed the ptzianos:rabbitmq_user_permission_fixes branch from a0e7ac8 to e6f97e0 Dec 2, 2018

@gundalow

This comment has been minimized.

Contributor

gundalow commented Dec 3, 2018

@ptzianos Thank you for this PR. Nice to see unit tests being added as well, could you please add a changelog/fragment to this PR https://docs.ansible.com/ansible/devel/community/development_process.html#changelogs Also if (in a separate PR) you'd like to add yourself as a maintainer in https://github.com/ansible/ansible/blob/devel/.github/BOTMETA.yml you will be notified of other rabbitmq PRs and have voting rights.

@groggi @windowsrefund @Xaroth I'd welcome your review on this

@ansibot ansibot removed the needs_triage label Dec 3, 2018

@Xaroth

This comment has been minimized.

Contributor

Xaroth commented Dec 3, 2018

Hm, upon further review, I think the set approach doesn't work with rabbitmq, as it returns dicts, @ptzianos , your list comprehension approach would work better (i.e. don't turn them into sets, but keep them as lists, and use your list comprehension).

@gundalow

This comment has been minimized.

Contributor

gundalow commented Dec 4, 2018

CI Failing with:

18:37 The full traceback is:
18:37 Traceback (most recent call last):
18:37   File "/root/.ansible/tmp/ansible-tmp-1543856207.88-12336870269511/AnsiballZ_rabbitmq_user.py", line 113, in <module>
18:37     _ansiballz_main()
18:37   File "/root/.ansible/tmp/ansible-tmp-1543856207.88-12336870269511/AnsiballZ_rabbitmq_user.py", line 105, in _ansiballz_main
18:37     invoke_module(zipped_mod, temp_path, ANSIBALLZ_PARAMS)
18:37   File "/root/.ansible/tmp/ansible-tmp-1543856207.88-12336870269511/AnsiballZ_rabbitmq_user.py", line 48, in invoke_module
18:37     imp.load_module('__main__', mod, module, MOD_DESC)
18:37   File "/tmp/ansible_rabbitmq_user_payload_0SGNjl/__main__.py", line 326, in <module>
18:37   File "/tmp/ansible_rabbitmq_user_payload_0SGNjl/__main__.py", line 319, in main
18:37   File "/tmp/ansible_rabbitmq_user_payload_0SGNjl/__main__.py", line 217, in set_permissions
18:37 TypeError: unhashable type: 'dict'

@gundalow gundalow added the ci_verified label Dec 4, 2018

@ptzianos ptzianos force-pushed the ptzianos:rabbitmq_user_permission_fixes branch from 52b8c75 to fc8f3fe Dec 7, 2018

@ptzianos

This comment has been minimized.

Contributor

ptzianos commented Dec 7, 2018

@Xaroth I dropped the commit with your suggestion. Thanks for the effort though.

@gundalow tests are passing now and I have nothing to add over here. I rebased on latest devel branch.

@gundalow gundalow merged commit a4eb4b2 into ansible:devel Dec 7, 2018

1 check passed

Shippable Run 97147 status is SUCCESS.
Details
@gundalow

This comment has been minimized.

Contributor

gundalow commented Dec 7, 2018

@ptzianos Thank you. Merged into devel for release in Ansible 2.8

@Xaroth Thank you for the reviews

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