Conversation
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
|
Thank you for your contribution! 🙏 Please understand that we will do our best to review your PR and give you feedback as soon as possible, but please bear with us if it takes a little longer as expected. While you are waiting, make sure to:
Once the initial tests are successful, a KEDA member will ensure that the e2e tests are run. Once the e2e tests have been successfully completed, the PR may be merged at a later date. Please be patient. Learn more about our contribution guide. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
/run-e2e kafka |
dttung2905
left a comment
There was a problem hiding this comment.
Thanks alot @rickbrouwer for this PR. Arguably, kafka scaler is the most complex out of all scalers in terms of the config logic and how each set interact with each other ( tls/ non-tls etc )
I do agree with the choices you made. We just need to add changes in the documentation stating that when tls, sasl, or saslTokenProvider are set in both ScaledObject and TriggerAuthentication, the ScaledObject (trigger metadata) value is used.
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
I've added a bit to the documentation (kedacore/keda-docs#1716). Not much, but it does clarify things. If you see anything different or want it changed, please let me know. You can also create a PR yourself so I can close mine; that's perfectly fine with me. Another question is that Microsoft is the maintainer of this scaler. What are your thoughts on kedacore/governance#132? Should we leave Microsoft as is, or should we change it to "Community"? |
|
/run-e2e kafka |
dttung2905
left a comment
There was a problem hiding this comment.
Thanks for the effort @rickbrouwer . all good with me now. I have also reviewed and approved the doc PR 🙏
Thanks @dttung2905, is appreciated! |
There was a problem hiding this comment.
Pull request overview
Refactors the Kafka scaler to use the shared typed-config parsing/validation approach and exposes Kafka trigger parameters in the generated scaler schema, aligning configuration handling and documentation across the project.
Changes:
- Refactor
kafka_scalermetadata parsing toScalerConfig.TypedConfigwith centralized validation (including partition limitation parsing). - Add Kafka scaler parameter definitions to the generated YAML/JSON schema outputs.
- Update Kafka scaler tests and changelog entry to reflect the refactor.
Reviewed changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| schema/generated/scalers-schema.yaml | Adds Kafka scaler parameter definitions to the generated schema. |
| schema/generated/scalers-schema.json | Adds the same Kafka scaler schema definitions in JSON form. |
| pkg/scalers/kafka_scaler.go | Migrates Kafka scaler metadata parsing to typed-config tags + Validate(), refactors TLS/SASL parsing, and updates field usage accordingly. |
| pkg/scalers/kafka_scaler_test.go | Updates/extends tests to match the new typed-config parsing behavior and adds partition limitation parsing tests. |
| CHANGELOG.md | Replaces placeholder entry with a Kafka scaler refactor note. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
|
/run-e2e kafka |
JorTurFer
left a comment
There was a problem hiding this comment.
AWESOME JOB! This probably was the worst scaler to migrate :(
Just one nit inline
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
|
/run-e2e kafka |
Long postponed, but finally done: the refactoring of the Kafka scaler.
The only thing that really deserves a comment and attention is the following choice I made. I have removed the exclusion checks for
tls,sasl, andsaslTokenProvider. Previously, explicitly setting these parameters in both ScaledObject and TriggerAuthentication simultaneously returned an error. With the migration to TypedConfig, this is no longer enforced, I have chosen that the value from triggerMetadata takes precedence. I did this to fully rely on TypedConfig, but also to reduce some boilerplate code.Further, I did add a changelog entry this time. I think it's worth mentioning, especially since the code now relies more heavily on the
enum, which wasn't the case before, and therefore the changes are subtle (though only if things are misconfigured).Checklist
make generate-scalers-schemahas been run to update any outdated generated filesRelates to #5797
Docs: kedacore/keda-docs#1716