Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Allow using simple queue names in aws sqs scaler #2483

Merged
merged 1 commit into from
Jan 24, 2022

Conversation

nicolaferraro
Copy link
Contributor

@nicolaferraro nicolaferraro commented Jan 14, 2022

AWS SQS autoscaler does not really need a full queueURL, it also works with simple queue names, but there was some code that tried to parse the URL to get the queue name for building a metric ID.

The example in the doc already uses a simple name.

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated

Fixes #

@nicolaferraro nicolaferraro requested a review from a team as a code owner January 14, 2022 08:56
@nicolaferraro nicolaferraro force-pushed the sqs-queueurl branch 3 times, most recently from 3bec0ca to 0f269f3 Compare January 14, 2022 14:52
@JorTurFer
Copy link
Member

JorTurFer commented Jan 16, 2022

Hi @nicolaferraro ,
Have you tried this? I feel that the PR will not work with simple queues because the function GetAwsSqsQueueLength still uses queueURL but probably I'm wrong (I don't know in depth about AWS SDK)

@tomkerkhove
Copy link
Member

I see you've checked " A PR is opened to update the documentation on (repo) (if applicable)" but I don't see a PR on https://github.com/kedacore/keda-docs/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc. Would you mind updating the docs for 2.6 please?

@nicolaferraro
Copy link
Contributor Author

Yes, I thought it wasn't required, but I've opened this: kedacore/keda-docs#628.

Sure, I've tested it manually and using the new integration I've created in Camel K.

Signed-off-by: nicolaferraro <ni.ferraro@gmail.com>
JorTurFer
JorTurFer previously approved these changes Jan 17, 2022
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks for your contribution ❤️

@JorTurFer JorTurFer requested a review from a team January 17, 2022 14:49
@tomkerkhove
Copy link
Member

Added concern on doc PR given we are re-using the queueURL name for a queue name: kedacore/keda-docs#628 (comment)

@JorTurFer JorTurFer dismissed their stale review January 17, 2022 14:58

Pending comments about naming in docs

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@zroubalik zroubalik added this to the v2.6.0 milestone Jan 24, 2022
@zroubalik zroubalik merged commit be3d926 into kedacore:main Jan 24, 2022
markrzasa pushed a commit to markrzasa/keda that referenced this pull request Jan 27, 2022
Signed-off-by: nicolaferraro <ni.ferraro@gmail.com>
Signed-off-by: Mark Rzasa <mark.rzasa@gmail.com>
markrzasa pushed a commit to markrzasa/keda that referenced this pull request Jan 27, 2022
Signed-off-by: nicolaferraro <ni.ferraro@gmail.com>
Signed-off-by: Mark Rzasa <mark.rzasa@gmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants