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

fix: Provide valid syntax for PostgreSQL queries #1282

Merged
merged 6 commits into from
Mar 8, 2024

Conversation

M4nihere
Copy link
Contributor

@M4nihere M4nihere commented Dec 21, 2023

There was a missing ";" in the triggers.metadata.query

Checklist

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

Fixes #
Just had to add a semicolon ";" at the end of the query.
image

Signed-off-by: Manish Kumar Mourya <[email protected]>
@M4nihere M4nihere requested a review from a team as a code owner December 21, 2023 18:00
Copy link

Thank you for your contribution! 🙏 We will review your PR as soon as possible.

While you are waiting, make sure to:

  • Add your contribution to all applicable KEDA versions
  • GitHub checks are passing
  • Is the DCO check failing? Here is how you can fix DCO issues

Learn more about:

Copy link

netlify bot commented Dec 21, 2023

Deploy Preview for keda ready!

Name Link
🔨 Latest commit 4f646ab
🔍 Latest deploy log https://app.netlify.com/sites/keda/deploys/65ea94f66e080900088539ba
😎 Deploy Preview https://deploy-preview-1282--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.

@JorTurFer
Copy link
Member

Hello @M4nihere ,
Thanks for your contribution! 🙇

I have a question, is the semicolon required or just for styling? I'm checking current e2e test and it doesn't have it
image

@M4nihere
Copy link
Contributor Author

Yes, there is always semicolon required at the end of a postgresql query, if you don't use semicolon the query will not return any value.

@M4nihere
Copy link
Contributor Author

M4nihere commented Dec 21, 2023

As you can see here below screenshot, query without semicolon is not giving any output.

image

Screenshot with semicolon!!
image

@JorTurFer
Copy link
Member

lol, I don't know how KEDA does the magic but it works without it (but I agree with adding it if it's the standard).
Could you update other versions as well with the semicolon?

@M4nihere
Copy link
Contributor Author

I myself have tested it on Keda ScaledObject with airflow.
So to thing was that the query was not returning any value so that it can scale the Statefulsets.
While troubleshooting I found that the semicolon is required.

Thanks !!

@M4nihere M4nihere closed this Dec 21, 2023
@M4nihere M4nihere reopened this Dec 21, 2023
@JorTurFer
Copy link
Member

Yeah, don't worry. I've tested with latest KEDA version and maybe something has changed, but in any case it works too, so we can add the semicolon to docs, np
Could you update other versions as well with the semicolon?

@M4nihere
Copy link
Contributor Author

Yes, I can do that but as you have mentioned in some Keda version it worked without semicolon, however I'm not sure how did that worked but I won't be able to test every Keda Version.

Because I'm using This version that's why I've found that bug.

@JorTurFer
Copy link
Member

JorTurFer commented Dec 21, 2023

Are you using KEDA v2.12, right?
I ask for executing the e2e test using your same version to double-check it

Let's update all

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.

@M4nihere thanks! Could you please fix this also for the other versions (including 2.13)?

Signed-off-by: Manish Kumar Mourya <[email protected]>
@M4nihere
Copy link
Contributor Author

@JorTurFer Just updated the 2.13 !!

@M4nihere
Copy link
Contributor Author

@JorTurFer Are you going to merge this pull request ?

@JorTurFer
Copy link
Member

@M4nihere thanks! Could you please fix this also for the other versions (including 2.13)?

Could you update older versions please? Then we can merge this :)

@M4nihere
Copy link
Contributor Author

Sure @JorTurFer

@tomkerkhove tomkerkhove changed the title Update postgresql.md fix: Provide valid syntax for PostgreSQL queries Jan 2, 2024
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Thank you! Mind updating all versions please?

@M4nihere M4nihere closed this Mar 8, 2024
@M4nihere M4nihere reopened this Mar 8, 2024
@M4nihere
Copy link
Contributor Author

M4nihere commented Mar 8, 2024

Hi @JorTurFer
I have updated all the previous version with correct postgresql command.
You can merge it now.

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.

Thanks 👍

@zroubalik zroubalik merged commit 900a3fe into kedacore:main Mar 8, 2024
8 checks passed
@ashb
Copy link

ashb commented Mar 25, 2024

This is just stylistic btw. A semi colon is required when using psql, but not needed when using a DB API client library to issue queries.

And you could make the argument that it's possible harmful in KEDA as it might mistakenly lead users to think they could issue multiple queries when only one will work.

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.

5 participants