-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix incorrect 'unmatched input property' notification #7176
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
Conversation
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. |
6f408fa
to
81bc176
Compare
/run-e2e loki|datadog|pulsar |
Signed-off-by: Rick Brouwer <[email protected]>
81bc176
to
c83eb54
Compare
/run-e2e loki|datadog|pulsar |
Signed-off-by: Rick Brouwer <[email protected]>
Signed-off-by: Rick Brouwer <[email protected]>
/run-e2e loki|datadog|pulsar |
Thanks for the review, @wozniakjan . Given the mentioned issue and the number of scalers likely affected by the various issues, I'd really like to nominate this PR for a patch release ( |
Signed-off-by: Rick Brouwer <[email protected]>
Apparently more scalers have been affected. I need to find out what the additional issue is there. |
Signed-off-by: Rick Brouwer <[email protected]>
Signed-off-by: Rick Brouwer <[email protected]>
Signed-off-by: Rick Brouwer <[email protected]>
Signed-off-by: Rick Brouwer <[email protected]>
9615f37
to
f47425f
Compare
Signed-off-by: Rick Brouwer <[email protected]>
/run-e2e datadog |
good idea, I think we can get 2.18.1 out as soon as the regressions are addressed, feel free to add more items to #7179 |
Agree, thanks for fixing this! |
Okay, I've checked the latest report regarding the Prometheus scaler in the issue. In that case, the report is "correct." I've marked the PR as "ready for review" again. |
By the way, the message now is:
I personally find that message strange. I would rather expect the type scaler, such as 'prometheus', so that the message is
I would therefore like to change:
Agree? |
yeah, the original one is obviously a mistake I would like to really thank you for fixing this issue that quickly! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes incorrect 'unmatched input property' notifications that were being generated in three different scenarios: handling authentication configurations that use *authentication.AuthMeta
, properties with multiple values (like Datadog's targetValue
/queryValue
), and scalers with parameters outside TypedConfig when also using TypedConfig.
Key changes:
- Modified typed configuration matching to return all parameter names instead of just the primary name
- Updated Loki scaler to use the new authentication config pattern
- Added
AuthModes
field to Pulsar scaler for better authentication handling - Refactored Datadog scaler to use unified TypedConfig approach with separate validation functions
Reviewed Changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
pkg/scalers/scalersconfig/typed_config.go | Fixed parameter name matching to return all names instead of just primary name |
pkg/scalers/loki_scaler.go | Migrated from AuthMeta to authentication.Config pattern |
pkg/scalers/pulsar_scaler.go | Added AuthModes field to support authentication configuration |
pkg/scalers/datadog_scaler.go | Refactored to use unified TypedConfig with separate validation functions |
tests/scalers/pulsar/helper/helper.go | Fixed test to use correct parameter name |
pkg/scalers/scalersconfig/typed_config_test.go | Updated test to handle unordered message validation |
pkg/scalers/loki_scaler_test.go | Updated test to use new authentication methods |
pkg/scalers/datadog_scaler_test.go | Refactored tests to use helper function for metadata creation |
CHANGELOG.md | Added entry for the fix and reordered existing entries |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also include the triggerName in the message (in case it is set) |
Signed-off-by: Rick Brouwer <[email protected]>
Co-authored-by: Copilot <[email protected]> Signed-off-by: Rick Brouwer <[email protected]>
/run-e2e loki|datadog|pulsar|prometheus |
Yeah, I like that. I have changed it to: Without triggerName: |
awesome fix! thanks for the quick reaction 🙇 |
This PR fixes the 'unmatched input property' notification which in some cases is wrongly given.
There are currently 3 different cases when this can occur:
The matching can't handle some authModes by (
*authentication.AuthMeta
). Loki and Pulsar uses this. I've changed Loki to the new method. I've done a quick fix for Pulsar. Changing toauthentication.Config
for Pulsar and making it backwards compatible is a significant refactoring effort.The matching can't handle properties that can have multiple values. Like in the Datadog scaler which has
targetValue
andqueryvalue
, like:useClusterAgentProxy
).And a little bonus;
msgBacklog
formsgBacklogThreshold
in the Pulsar test becausemsgBacklog
is unmatched.CHANGELOG.md
is because the order is not correct and the static checks are falling over itChecklist
Fixes #7174
Relates to #6763