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

Bugfix : Fix RabbitMQ policy definition change check #51562

Merged

Conversation

@gilbrechbuhler
Copy link
Contributor

commented Feb 8, 2019

What does this PR do?

Fix the way the rabbitmq_policy.present state compares the existing policy definition to the given policy definition to define the changes to apply.

As this field is a json field, compare the dict objects generated by json.loads instead of doing a string comparison.

What issues does this PR fix or reference?

Could not find any related issue.

Previous Behavior

An example state is defined like the following :

rabbit_policy:
  rabbitmq_policy.present:
    - name: HA
    - pattern: '.*'
    - definition: '{"ha-mode": "all"}'

Now, salt "*saltmaster*" rabbitmq.list_policies returns the policies for the vhost /, with the previous policy defined it returns for the policy definition :

definition:
                {"ha-mode":"all"}

So, in the current state of the code, the definition from the state and the definition returned by list_policies are compared as string, and due to the space that is removed between the property name and its value, the state acts like there was indeed a change in the definition.

New Behavior

Policy definitions are compared as dictionnaries and not as strings, avoiding uneeded re-application of existing policy.

Tests written?

No

Commits signed with GPG?

No

@dwoz

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

@gilbrechbuhler This works for old and new versions of RabbitMQ?

@gilbrechbuhler

This comment has been minimized.

Copy link
Contributor Author

commented Feb 14, 2019

@dwoz From what I can see, policies exist since version 3.0.0 of RabbitMQ (https://www.rabbitmq.com/changelog.html). So I tested with version 3.0 and it works perfectly. I also tested with the last released version of RabbitMQ (3.6.15) and it also works as expected.

Do you want me to run more extensive tests ?

@dwoz

This comment has been minimized.

Copy link
Contributor

commented Feb 14, 2019

@gilbrechbuhler No, I think this is okay. In the future though, you should create an issue for the bug before starting the work to fix it. :) It just helps us track these types of things and prevent regressions. Thanks for the contribution!

@dwoz dwoz merged commit bcc13db into saltstack:2017.7 Feb 14, 2019
10 checks passed
10 checks passed
WIP Ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint Python lint test has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.