Added authentication support for AQMP broker url #8057
Conversation
|
|
|
Reference issue raised |
|
Thanks for submitting a pull request 🚀 @koaning will take a look at it as soon as possible ✨ |
|
can some one verify it ? or provide an ETA ? |
|
Ah, it seems that I was notified of this during my holiday. The tagger shouldn't have tagged me in this case. I'll poke folks on slack to have a look right away. This is part of the codebase that I'm not familiar with so I prefer to have somebody else review. |
There was a problem hiding this comment.
Hey @nakshathru!
Thanks for your contribution and sorry that it took us so long to actually process and review it. I did the review today and left two smallish comments — could you please update your PR based on the information in this comments? Thanks a lot in advance!
rasa/core/brokers/pika.py
Outdated
| @@ -168,7 +169,9 @@ async def _connect(self) -> aio_pika.RobustConnection: | |||
| # The `url` parameter will take precedence over parameters like `login` or | |||
| # `password`. | |||
| if self.host.startswith("amqp"): | |||
| url = self.host | |||
|
|
|||
| host_ob = urlparse(self.host) | |||
There was a problem hiding this comment.
What does the name host_ob mean in this context? Can there be a better name for this?
There was a problem hiding this comment.
@alwx
I almost lost hope of you reviewing this PR after 6 months!!!!
Thanks for the update.
I renamed host_ob to parsed_host, does that make sense for you?
This object is just for holding the parsed url components values.
CHANGELOG.mdx
Outdated
| @@ -3924,3 +3924,4 @@ Regression: changes from `1.2.12` were missing from `1.4.0`, readded them | |||
|
|
|||
| * utterance templates defined in actions are checked for existence upon training a new agent, and a warning | |||
| is thrown before training if one is missing | |||
| * added authentication support for rabbitmq url which was missing before | |||
There was a problem hiding this comment.
We don't usually change old changelog entries — the new one needs to be added instead. This guide contains the detailed information about the process: https://github.com/RasaHQ/rasa/blob/main/changelog/README.md
But to keep it short: could you please create a 8057.improvement.md file inside a changelog directory with all the information about what was done in this PR instead of modifying CHANGELOG.mdx?
There was a problem hiding this comment.
@alwx Sounds good!! removed change log entry and created new one as per your suggestion
Co-authored-by: Alexander Pantiukhov <[email protected]>
Co-authored-by: Alexander Pantiukhov <[email protected]>
|
@alwx Are we good now? |
|
@nakshathru approved and ran all the tests. |
|
@nakshathru Ok, now we have the code quality issue (https://github.com/RasaHQ/rasa/pull/8057/checks?check_run_id=3369311492) — you can resolve it by running |
|
@nakshathru just let me know if you still want to continue working on this and fix the remaining code style issues. Otherwise, I would step in and update the code style. |
Head branch was pushed to by a user without write access
|
@alwx Thanks for the reminder. I tried |
|
@nakshathru thanks for your contribution! |
Updated AQMP url to include username, password and port params from the configuration
Issue #8042