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

Update trigger specification section for RabbitMQ, update 'host' parameter format, to specify that it supports vhost along with subpaths too #1146

Merged
merged 5 commits into from
Sep 12, 2023
Merged

Update trigger specification section for RabbitMQ, update 'host' parameter format, to specify that it supports vhost along with subpaths too #1146

merged 5 commits into from
Sep 12, 2023

Conversation

AmorBielyi
Copy link
Contributor

@AmorBielyi AmorBielyi commented Jun 7, 2023 •
edited
Loading

Update trigger specification section for RabbitMQ, update 'host' parameter format, to specify that it supports vhost along with subpaths too

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO)

Relates to kedacore/keda#2634
Relates to kedacore/keda#4584

@AmorBielyi
Update trigger specification section for RabbitMQ, update 'host' and …
559d41f 
…'vhostName' parameters format, to specify that it supports vhost along with subpaths too

Signed-off-by: Roman Bielyi <romanbielyi.amor@gmail.com>
@AmorBielyi AmorBielyi requested a review from a team as a code owner June 7, 2023 13:28
@netlify
Copy link

netlify bot commented Jun 7, 2023 •
edited
Loading

✅ Deploy Preview for keda ready!

Name Link
🔨 Latest commit 71193de
🔍 Latest deploy log https://app.netlify.com/sites/keda/deploys/64ff1a951c9d9b0008019504
😎 Deploy Preview https://deploy-preview-1146--keda.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@AmorBielyi AmorBielyi mentioned this pull request Jun 7, 2023
Closed
@tomkerkhove
Copy link
Member

I get the idea, but doesn't it make it a bit complex now? Maybe we just need to clarify in text instead of the current approach

@AmorBielyi
Copy link
Contributor Author

I get the idea, but doesn't it make it a bit complex now? Maybe we just need to clarify in text instead of the current approach

Hi, thank you for your review, could you please clarify which exactly text I need to change?

@tomkerkhove
Copy link
Member

I find this to be somewhat confusing: <protocol>://<host>:<port>/[path/.../pathN]vhost.

What do you think @JorTurFer @zroubalik?

@AmorBielyi
Copy link
Contributor Author

AmorBielyi commented Jun 8, 2023 •
edited
Loading

I find this to be somewhat confusing: <protocol>://<host>:<port>/[path/.../pathN]vhost.

What do you think @JorTurFer @zroubalik?

Thanks, what do you think about this? <protocol>://<host>:<port>/path/vhost

@tomkerkhove
Copy link
Member

<protocol>://<host>:<port>/path/vhost or <protocol>://<host>:<port>/<path>/vhost?

@AmorBielyi
Copy link
Contributor Author

<protocol>://<host>:<port>/path/vhost or <protocol>://<host>:<port>/<path>/vhost?

The last one looks good to me

@tomkerkhove
Copy link
Member

DoThisGIF

@AmorBielyi AmorBielyi changed the title Update trigger specification section for RabbitMQ, update 'host' and 'vhostName' parameters format, to specify that it supports vhost along with subpaths too Update trigger specification section for RabbitMQ, update 'host' parameter format, to specify that it supports vhost along with subpaths too Jun 8, 2023
@rtnpro
Copy link
Contributor

rtnpro commented Jun 13, 2023

@tomkerkhove What about also documenting a host URI containing a subpath(s) without vhost? In that case, the subpath should end with a trailing /. That's what is specified in the upstream docs: https://www.rabbitmq.com/management.html#path-prefix

image

@tomkerkhove
Copy link
Member

That would be fine for me as well

@AmorBielyi
Copy link
Contributor Author

AmorBielyi commented Jul 4, 2023 •
edited
Loading

@tomkerkhove What about also documenting a host URI containing a subpath(s) without vhost? In that case, the subpath should end with a trailing /. That's what is specified in the upstream docs: https://www.rabbitmq.com/management.html#path-prefix

image

Good idea! thanks, done! c9b509e

@AmorBielyi
Specify the new RabbitMQ connection string format more properly.
6a268ed 
Signed-off-by: Roman Bielyi <romanbielyi.amor@gmail.com>
@AmorBielyi
Specify that trailing slash is required if RabbitMQ host string doesn…
aedb7fb 
…'t contain vhost.

Signed-off-by: Roman Bielyi <romanbielyi.amor@gmail.com>
@AmorBielyi
Update trigger specification for RabbitMQ, specify that vhost + path …
4fb6955 
…is allowed only for HTTP protocol, for AMQP and AMQPS format remains the same.

Signed-off-by: Roman Bielyi <romanbielyi.amor@gmail.com>
@AmorBielyi
Copy link
Contributor Author

AmorBielyi commented Sep 11, 2023 •
edited
Loading

@tomkerkhove @JorTurFer Hi, could you please review this PR related to already merged kedacore/keda#4584, thanks in advance!

@JorTurFer
Copy link
Member

nice catch, I missed it 🙇

@JorTurFer JorTurFer merged commit 8c5c1f6 into kedacore:main Sep 12, 2023
9 checks passed
@JorTurFer JorTurFer mentioned this pull request Sep 12, 2023
Merged
@JorTurFer
Copy link
Member

okey... it seems that I'm blind :/
The changes were in the wrong file, this PR updates them: #1220

@AmorBielyi
Copy link
Contributor Author

AmorBielyi commented Sep 12, 2023 •
edited
Loading

okey... it seems that I'm blind :/ The changes were in the wrong file, this PR updates them: #1220

ouch! I apologize for this.. thanks for the quick fix, okay

@JorTurFer
Copy link
Member

No worries, It was my fail

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JorTurFer JorTurFer JorTurFer approved these changes

@tomkerkhove tomkerkhove Awaiting requested review from tomkerkhove

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AmorBielyi @tomkerkhove @rtnpro @JorTurFer