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 Scaler, allow subpaths along with vhost in connection string #4584

Merged
merged 4 commits into from
Sep 5, 2023
Merged

Fix RabbitMQ Scaler, allow subpaths along with vhost in connection string #4584

merged 4 commits into from
Sep 5, 2023

Conversation

AmorBielyi
Copy link
Contributor

@AmorBielyi AmorBielyi commented May 29, 2023 •
edited
Loading

Allow subpaths along with vhost in the RabbitMQ HTTP host string.
Extract them from the URL using string operations.

Checklist

Fixes #2634

@AmorBielyi AmorBielyi requested a review from a team as a code owner May 29, 2023 13:32
@AmorBielyi AmorBielyi changed the title Issues/2634 Added optional boolean field 'allowSubpathsOnHost' to the RabbitMQ scaler yaml config, in order to allow/disallow subpaths in the RabbitMQ connection string May 30, 2023
JorTurFer
JorTurFer reviewed May 30, 2023
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the improvement ❤️
Could you add some unit test to check this new parameter?
Additionally, this new parameter needs to be documented in keda-docs repository

CHANGELOG.md Outdated Show resolved Hide resolved
@AmorBielyi
Copy link
Contributor Author

Thanks for the improvement ❤️ Could you add some unit test to check this new parameter? Additionally, this new parameter needs to be documented in keda-docs repository

Hi, sure, thanks for the quick review and response from you ❤️ I will do it ASAP

@rtnpro rtnpro mentioned this pull request Jun 6, 2023
Closed
@AmorBielyi AmorBielyi changed the title Added optional boolean field 'allowSubpathsOnHost' to the RabbitMQ scaler yaml config, in order to allow/disallow subpaths in the RabbitMQ connection string Allow subpaths along with vhost in the RabbitMQ connection string. Extract them from the URL using string operations Jun 7, 2023
@AmorBielyi AmorBielyi requested a review from JorTurFer June 7, 2023 13:41
@AmorBielyi AmorBielyi mentioned this pull request Jun 7, 2023
Merged
1 task
@rtnpro
Copy link
Contributor

rtnpro commented Jun 7, 2023

Changes look good to me @AmorBielyi .

Did you test the above changes on a RabbitMQ setup with pathPrefix?

Ref: https://github.com/kedacore/sample-go-rabbitmq

@AmorBielyi
Copy link
Contributor Author

Changes look good to me @AmorBielyi .

Did you test the above changes on a RabbitMQ setup with pathPrefix?

Ref: https://github.com/kedacore/sample-go-rabbitmq

Not yet. I will test it today

@rtnpro
Copy link
Contributor

rtnpro commented Jun 13, 2023

@AmorBielyi Could you get the changes work with the amqp protocol? What I realized during my test is that the amqp go library does not support subpath in host URI at all.

https://github.com/kedacore/keda/blob/main/vendor/github.com/rabbitmq/amqp091-go/uri.go#L34

@JorTurFer
Copy link
Member

Rebase main branch please, there are some required changes for e2e tests

@AmorBielyi
Copy link
Contributor Author

Changes look good to me @AmorBielyi .

Did you test the above changes on a RabbitMQ setup with pathPrefix?

Ref: https://github.com/kedacore/sample-go-rabbitmq

Yes, tested

@AmorBielyi AmorBielyi changed the title Allow subpaths along with vhost in the RabbitMQ connection string. Extract them from the URL using string operations Allow subpaths along with vhost in the RabbitMQ HTTP host string. Extract them from the URL using string operations Jul 4, 2023
@AmorBielyi
Copy link
Contributor Author

AmorBielyi commented Jul 4, 2023 •
edited
Loading

@JorTurFer @rtnpro A bit refactored strings operations logic in getVhostAndPathFromURL function + refactored and improved unit tests according to these new changes. Would be grateful for your review.

@AmorBielyi
Copy link
Contributor Author

AmorBielyi commented Jul 4, 2023 •
edited
Loading

@AmorBielyi Could you get the changes work with the amqp protocol? What I realized during my test is that the amqp go library does not support subpath in host URI at all.

https://github.com/kedacore/keda/blob/main/vendor/github.com/rabbitmq/amqp091-go/uri.go#L34

I suppose we don't need to support subpaths for amqp case, it's only necessary for RabbitMQ Management HTTP API

@JorTurFer JorTurFer changed the base branch from release/v2.10 to main July 5, 2023 20:52
@JorTurFer JorTurFer changed the base branch from main to release/v2.10 July 5, 2023 20:53
JorTurFer
JorTurFer reviewed Jul 5, 2023
View reviewed changes
Copy link
Member

@JorTurFer JorTurFer left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Could you update your PR? it's done against release/v2.10 instead of main
image

pkg/scalers/rabbitmq_scaler.go Show resolved Hide resolved
semgrep-app[bot]
semgrep-app bot reviewed Jul 18, 2023
View reviewed changes
.github/workflows/fossa.yml Outdated Show resolved Hide resolved
semgrep-app[bot]
semgrep-app bot reviewed Jul 18, 2023
View reviewed changes
.github/workflows/fossa.yml Outdated Show resolved Hide resolved
@AmorBielyi
Copy link
Contributor Author

AmorBielyi commented Jul 18, 2023 •
edited
Loading

Could you update your PR? it's done against release/v2.10 instead of main image

sure, done, could you check please? @rtnpro @JorTurFer

@JorTurFer
Copy link
Member

it's still against the wrong branch:
image

To update the branch:
image
image

Once the branch is updated, the PR will calculate the code agains main branch

@AmorBielyi AmorBielyi changed the base branch from release/v2.10 to main August 25, 2023 10:55
@AmorBielyi
Copy link
Contributor Author

AmorBielyi commented Aug 25, 2023 •
edited
Loading

it's still against the wrong branch: image

To update the branch: image image

Once the branch is updated, the PR will calculate the code agains main branch

Hi, thanks! sorry about this, I've done, the branch now is 'main', all conflicts were resolved, unit tests for RabbitMQ Scaler passed, so let me know when you finished with review, please

JorTurFer
JorTurFer reviewed Sep 3, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

Could you also resolve DCO and merge conflicts? 🙏
image

@AmorBielyi
Copy link
Contributor Author

Could you also resolve DCO and merge conflicts? 🙏 image

For sure!

@JorTurFer
Copy link
Member

There are still some style issues + DCO :(

@AmorBielyi
Copy link
Contributor Author

There are still some style issues + DCO :(
Yeah.. I know, thanks) I am trying to resolve them now

@AmorBielyi AmorBielyi changed the title Allow subpaths along with vhost in the RabbitMQ HTTP host string. Extract them from the URL using string operations Fix RabbitMQ Scaler, allow subpaths along with vhost in connection string Sep 5, 2023
@AmorBielyi
Copy link
Contributor Author

AmorBielyi commented Sep 5, 2023 •
edited
Loading

@JorTurFer Hi,

I resolved DCO issue but in a bit different way, I followed these tips https://github.com/kedacore/keda/blob/main/CONTRIBUTING.md#i-didnt-sign-my-commit-now-what,
Is it okay? yeah ... I now and understand that I squashed commits into a single one, I am apologizing in advance if it's not a correct approach, and don't worry I have a copy of this branch before squashing, of course.

Also, could you please help me with failed CI Static Check (changelog-check), you said earlier I need to merge the main branch into this one, because main already has a fix for it, I did it, but it's not working, as you can see the Static Check for changelog has been failed.

JorTurFer
JorTurFer reviewed Sep 5, 2023
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member

Is it okay?

That's perfect. If DCO is solved, np at all

During the rebase something hasn't gone well, because the changelog isn't correct. The other changes than yours are wrong. If you check the original changelog, they aren't there :/
Could you just remove them? I hope that it's enough 🤞

@AmorBielyi
Fix CHANGELOG.md, remove redundant/wrong pieces that were made during…
a7d78e1 
… rebase, this fix is aimed to resolve failed workflow's Static Check (validate-changelog) test

Signed-off-by: Roman Bielyi <[email protected]>
@AmorBielyi
Copy link
Contributor Author

@JorTurFer it looks like that all workflow's tests has been successfully passed

@JorTurFer
Copy link
Member

JorTurFer commented Sep 5, 2023 •
edited by github-actions bot
Loading

/run-e2e rabbit
Update: You can check the progress here

@JorTurFer JorTurFer enabled auto-merge (squash) September 5, 2023 18:24
auto-merge was automatically disabled September 5, 2023 20:49

Head branch was pushed to by a user without write access

@AmorBielyi
Copy link
Contributor Author

@JorTurFer oh, there was a simple conflict just right before an auto-merge, I resolved it right now so let's wait workflow's tests one more time, please enable auto-merge again

@JorTurFer JorTurFer enabled auto-merge (squash) September 5, 2023 20:56
@JorTurFer
Copy link
Member

/skip-e2e

@AmorBielyi
Copy link
Contributor Author

@JorTurFer great! could you please approve PR if everything is done

@JorTurFer
Copy link
Member

JorTurFer commented Sep 5, 2023 •
edited
Loading

I thought that I did it, sorry

@JorTurFer JorTurFer merged commit 0386b17 into kedacore:main Sep 5, 2023
19 checks passed
Adarsh-verma-14 pushed a commit to Adarsh-verma-14/keda that referenced this pull request Sep 6, 2023
@AmorBielyi @Adarsh-verma-14
Fix RabbitMQ Scaler, allow subpaths along with vhost in connection st…
2e6b221 
…ring (kedacore#4584)

* fix RabbitMQ Scaler, allow subpaths along with vhost in connection string (kedacore#4584)

Signed-off-by: Roman Bielyi <[email protected]>

* Fix CHANGELOG.md, remove redundant/wrong pieces that were made during rebase, this fix is aimed to resolve failed workflow's Static Check (validate-changelog) test

Signed-off-by: Roman Bielyi <[email protected]>

---------

Signed-off-by: Roman Bielyi <[email protected]>
jeevanragula pushed a commit to jeevanragula/keda that referenced this pull request Sep 6, 2023
@AmorBielyi @jeevanragula
Fix RabbitMQ Scaler, allow subpaths along with vhost in connection st…
7d6433b 
…ring (kedacore#4584)

* fix RabbitMQ Scaler, allow subpaths along with vhost in connection string (kedacore#4584)

Signed-off-by: Roman Bielyi <[email protected]>

* Fix CHANGELOG.md, remove redundant/wrong pieces that were made during rebase, this fix is aimed to resolve failed workflow's Static Check (validate-changelog) test

Signed-off-by: Roman Bielyi <[email protected]>

---------

Signed-off-by: Roman Bielyi <[email protected]>
@JorTurFer JorTurFer mentioned this pull request Sep 24, 2023
Closed
6 tasks
toniiiik pushed a commit to toniiiik/keda that referenced this pull request Jan 15, 2024
@AmorBielyi @toniiiik
Fix RabbitMQ Scaler, allow subpaths along with vhost in connection st…
6334f46 
…ring (kedacore#4584)

* fix RabbitMQ Scaler, allow subpaths along with vhost in connection string (kedacore#4584)

Signed-off-by: Roman Bielyi <[email protected]>

* Fix CHANGELOG.md, remove redundant/wrong pieces that were made during rebase, this fix is aimed to resolve failed workflow's Static Check (validate-changelog) test

Signed-off-by: Roman Bielyi <[email protected]>

---------

Signed-off-by: Roman Bielyi <[email protected]>
Signed-off-by: anton.lysina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@semgrep-app semgrep-app[bot] semgrep-app[bot] left review comments

@JorTurFer JorTurFer JorTurFer approved these changes

Assignees
No one assigned
Labels
skip-e2e
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RabbitMQ api hosted in a subpath is truncated to http://host/api/ when it shouldbe http://host/subpath/api
4 participants
@AmorBielyi @rtnpro @JorTurFer @keda-automation