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

fix broken rabbitmq list policies in rabbitmq version 3.7 #47557

Merged
merged 3 commits into from Jun 4, 2018

Conversation

Projects
None yet
2 participants
@L4rS6
Contributor

L4rS6 commented May 9, 2018

What does this PR do?

This PR fixes the list_policies module output in rabbitmq version 3.7 and later. In rabbitmqctl version 3.7 the order of the list_policies output has changed:

Version 3.6
/ ha-all all ^(?!amq[.]).* {"ha-mode":"all"} 0

Version 3.7
/ ha-all ^(?!amq[.]).* all {"ha-mode":"all"} 0

In fact that we need the rabbitmq version I used salt's pkg.version module to detect its version. Due to different package names on different OS (freebsd, ubuntu) this check has also been implemented.

Previous Behavior

On minions with rabbitmq version 3.7 the order of apply_to and pattern was wrong:

    ----------
    /:
        ----------
        ha-all:
            ----------
            apply_to:
                ^(?!amq[.]).*
            definition:
                {"ha-mode":"all"}
            pattern:
                all
            priority:
                0

New Behavior

Order has been fixed for apply_to and pattern:

    ----------
    /:
        ----------
        ha-all:
            ----------
            apply_to:
                all
            definition:
                {"ha-mode":"all"}
            pattern:
                ^(?!amq[.]).*
            priority:
                0

Tests written?

No

Commits signed with GPG?

No

@rallytime

Hi @L4rS6 - Thanks for submitting this change, and apologies for not responding sooner. I have one question.

In addition, the following tests are failing:

  • unit.modules.test_rabbitmq.RabbitmqTestCase.test_list_policies
  • unit.modules.test_rabbitmq.RabbitmqTestCase.test_policy_exists

Can you take a look at that?

@@ -828,24 +828,46 @@ def list_policies(vhost="/", runas=None):
python_shell=False)
_check_response(res)
output = res['stdout']
if __salt__['pkg.version']('rabbitmq-server'):

This comment has been minimized.

@rallytime

rallytime May 15, 2018

Contributor

Would using something like salt.utils.versions.LooseVersion be beneficial here?

This comment has been minimized.

@L4rS6

L4rS6 May 31, 2018

Contributor

@rallytime Yes, I took a look at that. I'm thinking about to split up the rabbitmq.py module to a freebsd_rabbitmq.py and a default rabbitmq.py module, because of the different package names. If I do it like this I can set the package name in the freebsd module and don't have to make an ugly if-else construct. At the moment I'm not sure if I should copy all functions into the freebsd_rabbitmq.py module. If I do this we have a lot of duplicate code (freebsd_rabbitmq.py and rabbitmq.py). Do you have an idea how to solve this nicely?

You're right, I should use LooseVersion to compare the versions.

This comment has been minimized.

@rallytime

rallytime May 31, 2018

Contributor

The if/else is fine, but to answer your question, we use the files in the utils directory to hold code that is used across multiple modules/states.

Lars Wagner and others added some commits Jun 4, 2018

@rallytime

Welcome @L4rS6! Thank you for fixing this and for writing these additional tests. ++

@rallytime rallytime merged commit 484d830 into saltstack:2018.3 Jun 4, 2018

6 of 9 checks passed

jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19524 — ABORTED
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5468 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25665 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17735 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23401 — SUCCESS
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10440 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22365 — SUCCESS
Details

@L4rS6 L4rS6 deleted the L4rS6:fix-broken-rabbitmq-list-policies branch Jun 4, 2018

@L4rS6

This comment has been minimized.

Contributor

L4rS6 commented Jun 4, 2018

@rallytime Will this change also be merged into the master branch? I just want to be sure that those changes will be in future releases as well.

@rallytime

This comment has been minimized.

Contributor

rallytime commented Jun 4, 2018

@L4rS6 Yep! It will be merged forward to develop. We've got some doc on that here, if you're interested.

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