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.

Sign up for GitHub

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

Jump to bottom

fix(rabbitmq_plugin): add node to module args #106

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

fix(rabbitmq_plugin): add node to module args #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a6d1c2
fix(rabbitmq_plugin): add node to module args
rockandska Oct 27, 2021
2ca7511
chore(changelog): add fragment
rockandska Nov 2, 2021
e129073
chore(rabbitmq_plugin): add example for node arg
rockandska Nov 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions changelogs/fragments/106-use-node-with-plugin.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
minor_changes:
- rabbitmq_plugin - add node arguments.
Copy link
Contributor

@Andersson007 Andersson007 Jan 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- rabbitmq_plugin - add node arguments.
- rabbitmq_plugin - add the ``node`` argument (https://github.com/ansible-collections/community.rabbitmq/pull/106).

14 changes: 14 additions & 0 deletions plugins/modules/rabbitmq_plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@
- Does not disable plugins that are not in the names list.
type: bool
default: "no"
node:
description:
- Erlang node name of the rabbit we wish to configure.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Erlang node name of the rabbit we wish to configure.
- Erlang node name of the rabbit we wish to configure.

Please also add a note that this use -n under the hood and since which version this is supported.
If users don't use this argument, everything should be OK in terms of backwards compatibility, right?

type: str
default: rabbit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
default: rabbit
default: rabbit
version_added: '1.2.0'

state:
description:
- Specify if plugins are to be enabled or disabled.
Expand Down Expand Up @@ -74,6 +79,12 @@
names: rabbitmq_peer_discovery_aws_plugin
state: enabled
broker_state: offline

- name: Enables plugin with custom node name
community.rabbitmq.rabbitmq_plugin:
names: rabbitmq_management
state: enabled
node: bunny
'''

RETURN = r'''
Expand Down Expand Up @@ -114,6 +125,8 @@ def __init__(self, module):
def _exec(self, args, force_exec_in_check_mode=False):
if not self.module.check_mode or (self.module.check_mode and force_exec_in_check_mode):
cmd = [self._rabbitmq_plugins]
if self.module.params['node']:
cmd.extend(['-n', self.module.params['node']])
rc, out, err = self.module.run_command(cmd + args, check_rc=True)
return out.splitlines()
return list()
Expand All @@ -139,6 +152,7 @@ def main():
arg_spec = dict(
names=dict(required=True, aliases=['name']),
new_only=dict(default='no', type='bool'),
node=dict(default='rabbit'),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just wondering, by specifying default='rabbit' here would this always populate self.module.params['node'] with rabbit regardless of what's in the playbook?

I was looking through the rabbitmq.server code on github, but, it seems quite time consuming to go back through all the versions looking for when -n was introduced to rabbitmqctl/rabbitmq-plugins...

I was wondering, can we guarantee backward compatibility by changing line 155 to:

node=dict(type='str')

At least then, the user can chose not to put node in their playbook at it will work as before. What do you think?

Copy link
Author

@rockandska rockandska Nov 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The option is here at least since rabbitmq:3.4.4 ( 11 February 2015 )

$ docker run --rm -ti --entrypoint /bin/bash rabbitmq:3.4.4
root@cfd69b2f4d2c:/# rabbitmq-plugins
Error: could not recognise command
Usage:
rabbitmq-plugins [-n <node>] <command> [<command options>] 

Commands:
    list [-v] [-m] [-E] [-e] [<pattern>]
    enable [--offline] [--online] <plugin> ...
    disable [--offline] [--online] <plugin> ...
    set [--offline] [--online] <plugin> ...


root@cfd69b2f4d2c:/#

At least then, the user can chose not to put node in their playbook at it will work as before. What do you think?

I personally prefer to have all modules working the same that having some specificities depending on the module

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking that. It appears that 3.4.4 is the earliest docker container.

I personally prefer to have all modules working the same that having some specificities depending on the module

The only risk with the proposed PR is that it will specify -n on the command line all the time as default specifies rabbit. In it self, that is fine but if someone is using an older than 3.4.4. version which does not support -n and they use the latest community.rabbitmq version a rabbitmq_plugin playbook will fail. I was just trying to avoid this potential backwards compatibility issue... although it maybe quite a small risk?

If we proceed as is, perhaps we consider adjusting the changelog fragment to include a message that this will be a breaking change if the users rabbitmq-plugins does not support the -n flag. How does that sound?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO all module should not force options if not mandatory but for an obscure reason -n seems forced in many rabbitmq module and is the only reason why I add it.
So it is your call here, but it will be more consistent to apply this choice to all modules in a future PR.

Off-topic : For an example, #35 broke my ansible role because the choice was made to force --online when this option is not mandatory and rabbitmq-plugins is able to do the choice itself.....

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Im0 / @Andersson007 : so, force or not force -n ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cognifloyd @Im0 @odyssey4me what do you think?

@rockandska I'm unfortunately not an expert in the context, so i can provide only general feedback.
I can say that if we're gonna introduce any breaking changes, it would be nice:

  1. to announce this in advance, i.e. that it's gonna happen in the next major release or so
  2. release the major version in some time after the announcement

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cognifloyd @Im0 @odyssey4me PTAL ^ Hope you folks have great holidays:)

state=dict(default='enabled', choices=['enabled', 'disabled']),
broker_state=dict(default='online', choices=['online', 'offline']),
prefix=dict(required=False, default=None)
Expand Down
14 changes: 14 additions & 0 deletions tests/integration/targets/rabbitmq_plugin/tasks/tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,20 @@
that:
- result is not changed

- name: Using custom node should fail [online]
rabbitmq_plugin:
name: "{{ plugin_name }}"
state: enabled
new_only: True
node: bunny
register: result
ignore_errors: yes

- assert:
that:
- "result is failed"
- "result.cmd == '/usr/sbin/rabbitmq-plugins -n bunny enable --online rabbitmq_top'"

always:
- name: Disable plugin
rabbitmq_plugin:
Expand Down