Skip to content

Refactor elasticsearch scaler config#6101

Merged
zroubalik merged 4 commits intokedacore:mainfrom
rickbrouwer:elastic
Sep 26, 2024
Merged

Refactor elasticsearch scaler config#6101
zroubalik merged 4 commits intokedacore:mainfrom
rickbrouwer:elastic

Conversation

@rickbrouwer
Copy link
Member

@rickbrouwer rickbrouwer commented Aug 24, 2024 •
edited
Loading

  • 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
Member Author

rickbrouwer commented Aug 25, 2024 •
edited
Loading

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
@rickbrouwer
Refactor elasticsearch scaler config
c8d137f 
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
wozniakjan
wozniakjan reviewed Sep 4, 2024
View reviewed changes
@rickbrouwer rickbrouwer force-pushed the elastic branch 7 times, most recently from 10d1879 to c5f4318 Compare September 5, 2024 05:59
@rickbrouwer rickbrouwer mentioned this pull request Sep 5, 2024
Merged
1 task
@rickbrouwer
Make seperator configurable
51a1765 
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
@rickbrouwer
Add splitWithSeparator func
7b59c85 
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
wozniakjan
wozniakjan approved these changes Sep 6, 2024
View reviewed changes
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

zroubalik
zroubalik reviewed Sep 18, 2024
View reviewed changes
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

@rickbrouwer
Merge branch 'main' into elastic
30eb25e 
Signed-off-by: rickbrouwer <rickbrouwer@gmail.com>
@rickbrouwer
Copy link
Member Author

rickbrouwer commented Sep 18, 2024

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?

@wozniakjan
Copy link
Member

wozniakjan commented Sep 25, 2024 •
edited by github-actions bot
Loading

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

wozniakjan
wozniakjan approved these changes Sep 25, 2024
View reviewed changes
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!

@zroubalik zroubalik merged commit a5e979a into kedacore:main Sep 26, 2024
@rickbrouwer rickbrouwer deleted the elastic branch September 30, 2024 14:04
rickbrouwer added a commit to rickbrouwer/keda that referenced this pull request Oct 2, 2024
@rickbrouwer
Refactor elasticsearch scaler config (kedacore#6101)
7451417 
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
Signed-off-by: rickbrouwer <rickbrouwer@gmail.com>
rickbrouwer added a commit to rickbrouwer/keda that referenced this pull request Oct 2, 2024
@rickbrouwer
Refactor elasticsearch scaler config (kedacore#6101)
32c828e 
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
Signed-off-by: rickbrouwer <rickbrouwer@gmail.com>
JorTurFer pushed a commit to JorTurFer/keda that referenced this pull request Oct 7, 2024
@rickbrouwer @JorTurFer
Refactor elasticsearch scaler config (kedacore#6101)
e6e50eb 
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
Signed-off-by: rickbrouwer <rickbrouwer@gmail.com>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
@wozniakjan wozniakjan mentioned this pull request Oct 21, 2024
Open
58 tasks
mpechner-akasa pushed a commit to nrichardson-akasa/keda that referenced this pull request Nov 29, 2024
@rickbrouwer @mpechner-akasa
Refactor elasticsearch scaler config (kedacore#6101)
7ba99ad 
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
Signed-off-by: rickbrouwer <rickbrouwer@gmail.com>
Signed-off-by: michael pechner <mike.pechner@akasa.com>
chiragbhatia8 pushed a commit to chiragbhatia8/keda that referenced this pull request Jan 9, 2025
@rickbrouwer @chiragbhatia8
Refactor elasticsearch scaler config (kedacore#6101)
f4e055c 
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
Signed-off-by: rickbrouwer <rickbrouwer@gmail.com>
Signed-off-by: Chirag Bhatia <chiragbhatia8@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zroubalik zroubalik zroubalik approved these changes

@wozniakjan wozniakjan wozniakjan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rickbrouwer @wozniakjan @zroubalik