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

Refactor elasticsearch scaler config #6101

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rickbrouwer
Copy link
Contributor

@rickbrouwer rickbrouwer commented Aug 24, 2024

  • Refactor the scaler to match the new metadata guidelines.
  • Because i deleted const defaultUnsafeSsl = false in the elasticseach_scaler.go, kafka_scaler.go is giving error pkg/scalers/kafka_scaler.go:354:19: undefined: defaultUnsafeSsl. That's why I added the const in kafka_scaler.go.
  • Extra check if both username and password are provided when addresses is used (also test added).
  • Made a new splitWithSeparator function so the separator is configurable.
  • The documentation states that the field index is comma separated. In the code it is semicolon. There is a separate PR created in the keda-docs repo (1465).

Checklist

Related: 5797
Docs: 1465

@rickbrouwer rickbrouwer requested a review from a team as a code owner August 24, 2024 07:22
@rickbrouwer rickbrouwer marked this pull request as draft August 24, 2024 07:22
@rickbrouwer rickbrouwer force-pushed the elastic branch 21 times, most recently from 3ae60fc to 7f53e6e Compare August 25, 2024 07:51
@rickbrouwer
Copy link
Contributor Author

rickbrouwer commented Aug 25, 2024

Hi @wozniakjan

I've also refactored the Elasticsearch scaler to match the new metadata guidelines. This one needs a good look at the code 🙏

This because TypedConfig support slices, but only the elemSeparator accepts ",". So i added code so you can also slice on more separators like ";" (another option is to add a new field tag parameter like separator so you can choose. That will probably create some rework for already refactored scalers, but i like to hear your opinion).

@rickbrouwer rickbrouwer changed the title WIP: Refactor elasticsearch scaler config Refactor elasticsearch scaler config Aug 25, 2024
@rickbrouwer rickbrouwer marked this pull request as ready for review August 25, 2024 08:13
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
Copy link
Member

@wozniakjan wozniakjan 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, very well done!

ptal @kedacore/keda-core-contributors

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.

Looking good, could you please resolve the config? then we are good to merge this one. Thanks

Signed-off-by: rickbrouwer <rickbrouwer@gmail.com>
@rickbrouwer
Copy link
Contributor Author

Looking good, could you please resolve the config? then we are good to merge this one. Thanks

Done :) Can you also run the run-e2e run for elasticsearch?

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.

3 participants