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

Fixing bug whereby a queue name with a slash in it resulted in a REST API error #144

Conversation

Im0
Copy link
Collaborator

@Im0 Im0 commented Oct 30, 2022

SUMMARY

Fixes #114 - If a queue name was specified with a / in it, this resulted in a REST API error. Also, took the chance to add a documentation example for providing arguments.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

rabbitmq_queue

ADDITIONAL INFORMATION

@Im0
Copy link
Collaborator Author

Im0 commented Oct 30, 2022

CI is failing on unrelated issue which is being worked on in #139

@cognifloyd @odyssey4me @jgkirschbaum if anyone could take a quick look/review it would be appreciated. Thanks :)

@Im0 Im0 changed the title Fixing bug whereby a queue name with a slash in it resulted in a REST API error WIP: Fixing bug whereby a queue name with a slash in it resulted in a REST API error Nov 1, 2022
@Im0
Copy link
Collaborator Author

Im0 commented Nov 1, 2022

TODO:
* tests/integration/targets/rabbitmq_publish/tasks/ubuntu.yml - unsure why this was deleted... will put back in.
* tests/integration/targets/rabbitmq_queue/tasks/ubuntu.yml - First task mentions pika... remove comment about pika.

@Im0 Im0 force-pushed the 114-queue-name-with-slash-community.rabbitmq.rabbitmq_queue branch from 969f5b5 to ef42923 Compare November 1, 2022 09:15
@Im0 Im0 closed this Nov 2, 2022
@Im0 Im0 reopened this Nov 2, 2022
Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

@Im0 thanks for the bugfix!

one formatting thing from me

@csmart @odyssey4me @cognifloyd do you have any opinions on the patch?

changelogs/fragments/114-queue-name-escape.yml Outdated Show resolved Hide resolved
@Im0
Copy link
Collaborator Author

Im0 commented Nov 2, 2022

Thanks @Andersson007 ! Other rabbitmq_* modules had this issue previously and appear to also have been corrected with urllib_parse.quote(module.params['name'], ''). With the additional tests in place this is low risk in my opinion. Hopefully we can get the tick from one more reviewer as I'd like to release v1.2.3 of the collection asap. :)

Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

@Im0 SGTM, thanks! if no objections, i think it's OK if you merge it tomorrow

@Andersson007
Copy link
Contributor

@Im0 so, as there are no objections, feel free to merge

@Im0
Copy link
Collaborator Author

Im0 commented Nov 3, 2022

Thanks @Andersson007

@Im0 Im0 changed the title WIP: Fixing bug whereby a queue name with a slash in it resulted in a REST API error Fixing bug whereby a queue name with a slash in it resulted in a REST API error Nov 3, 2022
@Im0 Im0 merged commit 1bd96f2 into ansible-collections:main Nov 3, 2022
@Im0 Im0 deleted the 114-queue-name-with-slash-community.rabbitmq.rabbitmq_queue branch November 3, 2022 11:24
@Andersson007
Copy link
Contributor

@Im0 thanks for the contribution!
Just a fyi if once you get bored with doing only rabbitmq stuff (though there's a lot at the moment), there are many other opportunities to contribute.
Besides other collections, see the Contributor Path for details.
If there are any question, I'd be always happy to clarify them here or via email.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to specify queue type in rabbitmq_queue module
2 participants