fix(rabbitmq_plugin): add node to module args #106
Conversation
rockandska
force-pushed
the
fix_node_plugin
branch
from
690639a
to
2a6d1c2
Compare
|
For an obscure reason, previously to ansible 2.12 if the node argument contain an I updated the test to remove |
There was a problem hiding this comment.
@rockandska hi, thanks for the PR and welcome to the project!
Could you also please
- Add a changelog fragment.
- Maybe it's worth adding an example to the EXAMPLES block?
- Clarify if this option works for any supported rabbitmq versions?
Thank you
|
@rockandska good one. Looks good to me. It would be great if you could add suggestions 1 & 2 from @Andersson007 |
rockandska
force-pushed
the
fix_node_plugin
branch
from
edddbc7
to
e129073
Compare
|
Done |
| @@ -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'), | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.....
There was a problem hiding this comment.
@Im0 / @Andersson007 : so, force or not force -n ?
There was a problem hiding this comment.
@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:
- to announce this in advance, i.e. that it's gonna happen in the next major release or so
- release the major version in some time after the announcement
There was a problem hiding this comment.
@cognifloyd @Im0 @odyssey4me PTAL ^ Hope you folks have great holidays:)
There was a problem hiding this comment.
@rockandska thanks for working on this and sorry for the delayed feedback - I'm not a specialist at all and I've hoped the folks had a look.
As @Im0 has taken part in the discussion and there were no objections from his side, please take a look at my suggestions and, I think, I'll merge the PR right after they are implemented.
Thank you!
| @@ -0,0 +1,2 @@ | |||
| minor_changes: | |||
| - rabbitmq_plugin - add node arguments. | |||
There was a problem hiding this comment.
| - rabbitmq_plugin - add node arguments. | |
| - rabbitmq_plugin - add the ``node`` argument (https://github.com/ansible-collections/community.rabbitmq/pull/106). |
| description: | ||
| - Erlang node name of the rabbit we wish to configure. | ||
| type: str | ||
| default: rabbit |
There was a problem hiding this comment.
| default: rabbit | |
| default: rabbit | |
| version_added: '1.2.0' |
| @@ -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. | |||
There was a problem hiding this comment.
| - 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?
SUMMARY
Add
nodeargument torabbitmq_pluginthe same way as other module ( close #59 )ISSUE TYPE
COMPONENT NAME
rabbitmq_plugin
ADDITIONAL INFORMATION
Integration test and unit test ok
Minimal test added
@odyssey4me / @Andersson007 : since #101 is still a WIP.
Regards,