Skip to content

Comments

refactor: separate test-only scalers from production code#7372

Open
dshmatov wants to merge 3 commits intokedacore:mainfrom
dshmatov:refactor/separate-test-scalers
Open

refactor: separate test-only scalers from production code#7372
dshmatov wants to merge 3 commits intokedacore:mainfrom
dshmatov:refactor/separate-test-scalers

Conversation

@dshmatov
Copy link

@dshmatov dshmatov commented Jan 8, 2026

Remove external-mock scaler from production buildScaler switch and implement a test scaler registry pattern instead.

Previously, the external-mock scaler (used only for testing) was defined in the production buildScaler() switch statement, mixing test and production code. This resolves the TODO comment at line 182.

Changes:

  • Remove external-mock case and TODO from buildScaler()
  • Add RegisterTestScalerBuilder() function for test-only scalers
  • Register external-mock in BeforeSuite via the registry pattern
  • Add unit tests to verify registry functionality

Checklist

  • When introducing a new scaler, I agree with the scaling governance policy
  • I have verified that my change is according to the deprecations & breaking changes policy
  • Tests have been added
  • Ensure make generate-scalers-schema has been run to update any outdated generated files.
  • Changelog has been updated and is aligned with our changelog requirements
  • A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #

Relates to #

Remove external-mock scaler from production buildScaler switch and
implement a test scaler registry pattern instead.

Previously, the external-mock scaler (used only for testing) was
defined in the production buildScaler() switch statement, mixing
test and production code. This resolves the TODO comment at line 182.

Changes:
- Remove external-mock case and TODO from buildScaler()
- Add RegisterTestScalerBuilder() function for test-only scalers
- Register external-mock in BeforeSuite via the registry pattern
- Add unit tests to verify registry functionality

Signed-off-by: Dmitriy Shmatov <dmitry.shmatov@wiliot.com>
Signed-off-by: Dmitriy Shmatov <shmatov.dmitriy@gmail.com>
@dshmatov dshmatov requested a review from a team as a code owner January 8, 2026 11:22
@github-actions
Copy link

github-actions bot commented Jan 8, 2026

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:

  • Add an entry in our changelog in alphabetical order and link related issue
  • Update the documentation, if needed
  • Add unit & e2e tests for your changes
  • GitHub checks are passing
  • Is the DCO check failing? Here is how you can fix DCO issues

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.

@keda-automation keda-automation requested a review from a team January 8, 2026 11:22
@snyk-io
Copy link

snyk-io bot commented Jan 8, 2026

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

Interesting approach! I'm thinking if we should register all the scaler builders as a map and just invoke them when needed. a map should be almost performant as a switch and it can remove all the testing code from the prod code registering the prod ones here and the extra one in the test file. WDYT @kedacore/keda-core-maintainers
@kedacore/keda-core-contributors

@dshmatov
Copy link
Author

dshmatov commented Jan 8, 2026

@JorTurFer yeah, good idea to replace this huge switch block! I can try to implement if everyone good with this approach

@keda-automation keda-automation requested a review from a team January 10, 2026 22:27
Implement full registry pattern as suggested in PR feedback:
- Replace 150-line switch statement with map-based registry
- Register all production scalers in init() function
- Unify test and production scaler registration

Benefits:
- Complete separation of test and production code
- Eliminates large switch statement from buildScaler()
- More extensible and maintainable architecture

All existing tests pass with no functional changes.

Signed-off-by: Dmitriy Shmatov <shmatov.dmitriy@gmail.com>
@dshmatov dshmatov force-pushed the refactor/separate-test-scalers branch from ca54bdc to 003d49a Compare January 10, 2026 22:38
Implement full registry pattern as suggested in PR feedback:
- Replace 150-line switch statement with map-based registry
- Register all production scalers in init() function
- Unify test and production scaler registration

Changes:
- Created scalers_registry.go with init-based registration
- Updated buildScaler() to use map lookup
- Updated schema generator to parse registry format
- Updated sort_scalers.sh to check registry file
- Updated Makefile to point to registry file
- Fixed unused parameter linter warnings

Benefits:
- Complete separation of test and production code
- Eliminates large switch statement
- More extensible and maintainable architecture

Signed-off-by: Dmitriy Shmatov <shmatov.dmitriy@gmail.com>
@dshmatov dshmatov requested a review from JorTurFer January 10, 2026 23:56
@dshmatov
Copy link
Author

I've implemented the full registry pattern - replaced the entire switch statement with map-based registration. All production scalers now register via init(), test scalers use the same RegisterScalerBuilder() API, and buildScaler() is now just a simple map lookup.

@rickbrouwer rickbrouwer added the merge-conflict This PR has a merge conflict label Jan 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-conflict This PR has a merge conflict

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants