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

Add documentation about the new values in trigger auth in PostgreSQL Scaler #535

Merged
merged 4 commits into from
Oct 4, 2021
Merged

Add documentation about the new values in trigger auth in PostgreSQL Scaler #535

merged 4 commits into from
Oct 4, 2021

Conversation

JorTurFer
Copy link
Member

Signed-off-by: jorturfer jorge_turrado@hotmail.es

Add documentation about the new values in trigger auth in PostgreSQL Scaler

Checklist

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

Related kedacore/keda#2114
Fixes kedacore/keda#2110

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
@@ -75,8 +75,12 @@ You can authenticate by using a password or store the password within the connec

**Password Authentication:**

- `password` - Password for configured user to login to postgreSQL database
variables.
- `host:` Service URL to postgresql. Note that you should use a full svc URL as KEDA will need to contact postgresql from a different namespace
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- `host:` Service URL to postgresql. Note that you should use a full svc URL as KEDA will need to contact postgresql from a different namespace
- `host:` Service URL to postgresql. Note that you should use a fully qualified URL -- including the namespace -- as KEDA will need to contact postgresql from a different namespace

Copy link
Member Author

Choose a reason for hiding this comment

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

Honestly, I have copied and pasted from the current "metadata definitions", so I will change both :)

Co-authored-by: Aaron Schlesinger <70865+arschles@users.noreply.github.com>

Signed-off-by: jorturfer <jorge_turrado@hotmail.es>
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.

Just needs a few improvements

@@ -22,7 +22,7 @@ Alternatively, a user can specify individual
arguments (host, userName, password, etc.), and the scaler will form a connection string
internally.

- `host:` Service URL to postgresql. Note that you should use a full svc URL as KEDA will need to contact postgresql from a different namespace
- `host:` Service URL to postgresql. Note that you should use a fully qualified URL -- including the namespace -- as KEDA will need to contact postgresql from a different namespace.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `host:` Service URL to postgresql. Note that you should use a fully qualified URL -- including the namespace -- as KEDA will need to contact postgresql from a different namespace.
- `host:` Service URL to PostgreSQL. Note that you should use a fully qualified URL (including the namespace) as KEDA will need to contact PostgreSQL from a different namespace.

@@ -22,7 +22,7 @@ Alternatively, a user can specify individual
arguments (host, userName, password, etc.), and the scaler will form a connection string
Copy link
Member

Choose a reason for hiding this comment

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

Nit - Can you ensure all values use the PostgreSQL casing please given it's a product name?

Copy link
Member

Choose a reason for hiding this comment

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

Same for the <name>: where the : needs to be moved out.

Copy link
Member

Choose a reason for hiding this comment

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

Actually let's align with the majority of scalers:

  • host - The hostname of the MSSQL instance endpoint.

Copy link
Member Author

@JorTurFer JorTurFer Sep 22, 2021

Choose a reason for hiding this comment

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

Do you mean using propery - description? Prometheus Scaler for example uses property: description also. WDYT if in another PR I review the scalers docs and unify the notation and style?
IMHO, unify the styles is out of the scope of this PR (but as I said, I'm opened to do it in another PR)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that notation.

A seperate PR would be awesome, if you are willing to do that of course.

Copy link
Member Author

@JorTurFer JorTurFer Sep 22, 2021

Choose a reason for hiding this comment

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

Sure, I will do :)
Maybe I can open a discussion to decide which should be the correct way because we have several ways in the scalers

Copy link
Member Author

Choose a reason for hiding this comment

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

I have opened it
#539

@@ -75,8 +75,12 @@ You can authenticate by using a password or store the password within the connec

**Password Authentication:**

- `password` - Password for configured user to login to postgreSQL database
variables.
- `host:` Service URL to postgresql. Note that you should use a fully qualified URL -- including the namespace -- as KEDA will need to contact postgresql from a different namespace.
Copy link
Member

Choose a reason for hiding this comment

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

See above

variables.
- `host:` Service URL to postgresql. Note that you should use a fully qualified URL -- including the namespace -- as KEDA will need to contact postgresql from a different namespace.
- `userName:` Username for postgresql user
- `password` Password for configured user to login to postgreSQL database variables.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `password` Password for configured user to login to postgreSQL database variables.
- `password`: Password for configured user to login to postgreSQL database.

- `userName:` Username for postgresql user
- `password` Password for configured user to login to postgreSQL database variables.
- `port:` Postgresql port
- `dbName:` Postgresql Database name
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `dbName:` Postgresql Database name
- `dbName`: Name of PostgreSQL database

- `host:` Service URL to postgresql. Note that you should use a fully qualified URL -- including the namespace -- as KEDA will need to contact postgresql from a different namespace.
- `userName:` Username for postgresql user
- `password` Password for configured user to login to postgreSQL database variables.
- `port:` Postgresql port
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `port:` Postgresql port
- `port`: Port of PostgreSQL database

- `password` - Password for configured user to login to postgreSQL database
variables.
- `host:` Service URL to postgresql. Note that you should use a fully qualified URL -- including the namespace -- as KEDA will need to contact postgresql from a different namespace.
- `userName:` Username for postgresql user
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `userName:` Username for postgresql user
- `userName`: Username for postgresql user

@@ -75,8 +75,12 @@ You can authenticate by using a password or store the password within the connec

**Password Authentication:**

- `password` - Password for configured user to login to postgreSQL database
variables.
- `host:` Service URL to postgresql. Note that you should use a fully qualified URL -- including the namespace -- as KEDA will need to contact postgresql from a different namespace.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `host:` Service URL to postgresql. Note that you should use a fully qualified URL -- including the namespace -- as KEDA will need to contact postgresql from a different namespace.
- `host`: Service URL to postgresql. Note that you should use a fully qualified URL -- including the namespace -- as KEDA will need to contact postgresql from a different namespace.

- `password` Password for configured user to login to postgreSQL database variables.
- `port:` Postgresql port
- `dbName:` Postgresql Database name
- `sslmode:` SSL policy for communicating with database
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- `sslmode:` SSL policy for communicating with database
- `sslmode`: SSL policy for communicating with database

Is the field sslmode or sslMode?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the connection string is sslMode but our trigger variable is sslmode

Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate on that please? You mean trigger metadata uses sslMode but trigger authentication uses sslmode?

Copy link
Member

Choose a reason for hiding this comment

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

No, the other way around probably?

Copy link
Member Author

Choose a reason for hiding this comment

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

The required key in the connection string is sslMode, but inside the scaler we are looking for sslmode when we are parsing the metadata. Our key is sslmode, we take the value from the metadata key sslmode and place in the connection string key sslMode

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can look for both when we are parsing the scaler metadata and deprecate sslmode in favor of sslMode (the last one is how we have to place in connection string)

Copy link
Member

Choose a reason for hiding this comment

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

If sslMode is only in the connection string then we can just keep on using sslmode as we do today, no need to check for sslMode field then as well

Copy link
Member Author

Choose a reason for hiding this comment

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

okey, so basically we can let it as it is :)

Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com>

Signed-off-by: Jorge Turrado <jorge.turrado@docplanner.com>
@zroubalik zroubalik requested a review from tomkerkhove October 4, 2021 08:25
@zroubalik zroubalik merged commit 468e258 into kedacore:master Oct 4, 2021
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.

Provide support for configuring authentication through TriggerAuthentication for PostgresSQL scaler
4 participants