K8s: Rollback Job scaling strategy default in chart value#3168
Open
VietND96 wants to merge 1 commit into
Open
Conversation
Signed-off-by: Viet Nguyen Duc <nguyenducviet4496@gmail.com>
Contributor
PR Summary by QodoHelm chart: default KEDA ScaledJob scalingStrategy by leaving strategy unset
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Contributor
Code Review by Qodo
1. Empty scaling strategy emitted
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
1197
to
1200
| scalingStrategy: | ||
| # -- Scaling strategy for KEDA ScaledJob - https://keda.sh/docs/latest/reference/scaledjob-spec/#scalingstrategy | ||
| strategy: "accurate" | ||
| strategy: "" | ||
| # -- Number of Completed jobs should be kept |
Contributor
There was a problem hiding this comment.
1. Empty scaling strategy emitted 🐞 Bug ≡ Correctness
charts/selenium-grid/values.yaml now defaults autoscaling.scaledJobOptions.scalingStrategy.strategy to "", and the autoscalingTemplate helper will still emit the non-empty scalingStrategy map into ScaledJob specs. This produces rendered ScaledJobs with scalingStrategy.strategy: "" (not an omitted field and not the repo’s documented/tested "default" strategy), so enabling autoscaling with defaults yields a misconfigured ScaledJob spec.
Agent Prompt
## Issue description
The chart default sets `autoscaling.scaledJobOptions.scalingStrategy.strategy` to an empty string, but the template emits the `scalingStrategy` object anyway, resulting in rendered ScaledJobs containing `scalingStrategy.strategy: ""`.
## Issue Context
The autoscaling template only filters empty values at the top level; nested maps like `scalingStrategy: {strategy: ""}` are considered non-empty and are rendered.
## Fix Focus Areas
- charts/selenium-grid/values.yaml[1196-1200]
- charts/selenium-grid/templates/_helpers.tpl[239-279]
- charts/selenium-grid/CONFIGURATION.md[457-460]
## Suggested fix
Either:
1) Set the chart default to the concrete strategy used by repo examples/CI (e.g., `strategy: "default"`), OR
2) If the intent is to rely on KEDA defaults, change defaults/templates so `scalingStrategy` is *omitted* when `strategy` is empty (e.g., default `scalingStrategy: {}` and/or explicitly `unset`/drop `scalingStrategy` when `.strategy` is empty, including nested cleanup).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| AUTOSCALING_COOLDOWN_PERIOD=${AUTOSCALING_COOLDOWN_PERIOD:-"1800"} | ||
| ENABLE_VIDEO_RECORDER=${ENABLE_VIDEO_RECORDER:-"true"} | ||
| SCALING_STRATEGY=${SCALING_STRATEGY:-"accurate"} | ||
| SCALING_STRATEGY=${SCALING_STRATEGY:-""} |
Contributor
There was a problem hiding this comment.
2. Tests override strategy empty 🐞 Bug ☼ Reliability
tests/charts/make/chart_test.sh now defaults SCALING_STRATEGY to "" and still always appends `--set
autoscaling.scaledJobOptions.scalingStrategy.strategy=${SCALING_STRATEGY}` when autoscaling is
enabled. This forces chart test runs to render an explicit empty strategy unless the environment
variable is set, making tests diverge from the repo’s reference values and intended defaults.
Agent Prompt
## Issue description
The test harness defaults `SCALING_STRATEGY` to an empty string and unconditionally passes it via `--set ...strategy=${SCALING_STRATEGY}` when autoscaling is enabled.
## Issue Context
Passing `--set key=` results in an explicit empty string value, overriding chart defaults and any values-file-provided strategy.
## Fix Focus Areas
- tests/charts/make/chart_test.sh[16-33]
- tests/charts/make/chart_test.sh[221-233]
- Makefile[1430-1435]
## Suggested fix
Only append `--set autoscaling.scaledJobOptions.scalingStrategy.strategy=...` when `SCALING_STRATEGY` is non-empty; otherwise omit the flag entirely so chart defaults/values files drive behavior. Optionally update the Makefile target to not set `SCALING_STRATEGY` at all unless explicitly requested.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| Settings that KEDA [ScaledJob spec](https://keda.sh/docs/latest/concepts/scaling-jobs/#scaledjob-spec) supports can be set via `autoscaling.scaledJobOptions`. | ||
|
|
||
| Expected that with default configuration in KEDA resource, autoscaling behavior should be correct. Hence, in chart values, we keep the config key `autoscaling.scaledJobOptions.scalingStrategy.strategy` is `accurate`. | ||
| Expected that with default configuration in KEDA resource, autoscaling behavior should be correct. Hence, in chart values, we keep the config key `autoscaling.scaledJobOptions.scalingStrategy.strategy` is `default`. |
Contributor
There was a problem hiding this comment.
3. Docs default inconsistent 🐞 Bug ⚙ Maintainability
README.md now claims the chart keeps autoscaling.scaledJobOptions.scalingStrategy.strategy as default, but CONFIGURATION.md and values.yaml list the default as an empty string. This leaves users without a single source of truth for what the chart actually renders by default.
Agent Prompt
## Issue description
README says the default strategy is `default`, but CONFIGURATION.md and values.yaml show `""`.
## Issue Context
This mismatch is user-facing and also conflicts with repo reference values that set `strategy: default`.
## Fix Focus Areas
- charts/selenium-grid/README.md[228-233]
- charts/selenium-grid/CONFIGURATION.md[457-460]
- charts/selenium-grid/values.yaml[1196-1200]
## Suggested fix
Make README, CONFIGURATION.md, and values.yaml all reflect the same default behavior (either a concrete `default` value, or explicitly document that the field is omitted/unset and ensure templates omit it).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it
Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.
Description
Motivation and Context
Types of changes
Checklist