Use empty strings as defaults for some empty values

[maxmanuylov]

User description

Description

Use empty strings as default values for object fields that templates iterate over.

Motivation and Context

Maybe it's a Helm bug, but the following code produces different results depending on the values of the ".nodeConfigMap.extraScripts" fields:

{{- range $fileName, $value := $.Values.nodeConfigMap.extraScripts }}
- name: {{ tpl (default (include "seleniumGrid.node.configmap.fullname" $) $.Values.nodeConfigMap.scriptVolumeMountName) $ }}
  mountPath: {{ $.Values.nodeConfigMap.extraScriptsDirectory }}/{{ $fileName }}
  subPath: {{ $fileName }}
{{- end }}

1. Installing selenium-grid chart itself, values are nil: correct output
2. Installing selenium-grid chart itself, values are "": correct output
3. Installing a chart containing selenium-grid chart as a dependency, values are nil: the block above produces empty output, looks like it performs no iterations
4. Installing a chart containing selenium-grid chart as a dependency, values are "": correct output

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

---

Type

bug_fix

---

Description

• This PR addresses an issue where Helm templates would not iterate over certain ConfigMap fields when they were nil, by setting their default values to empty strings.
• Specifically, it modifies nodeConfigMap.extraScripts, recorderConfigMap.extraScripts, and uploaderConfigMap.extraScripts in the values.yaml file of the selenium-grid chart to have empty string defaults for scripts like nodePreStop.sh, nodeProbe.sh, video.sh, video_graphQLQuery.sh, and upload.sh.
• This change ensures consistent behavior when the selenium-grid chart is installed directly or as a dependency of another chart, regardless of whether the values are nil or empty strings.

---

Changes walkthrough

Relevant files

Bug fix

values.yaml Set Default Empty String Values for ConfigMap Scripts        charts/selenium-grid/values.yaml Set default values for nodeConfigMap.extraScripts, recorderConfigMap.extraScripts, and uploaderConfigMap.extraScripts to empty strings. Ensured that scripts like nodePreStop.sh, nodeProbe.sh, video.sh, video_graphQLQuery.sh, and upload.sh have empty string defaults to avoid iteration issues in Helm templates. +5/-5

---

✨ PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

[qodo-merge-pro]

PR Description updated to latest commit (a98e39c)

• Copy walkthrough table to "Files Changed" Tab

[qodo-merge-pro]

PR Review

⏱️ Estimated effort to review [1-5]	2, because the changes are straightforward and limited to a few lines in a single file. The logic is simple: initializing script entries with empty strings instead of leaving them undefined or null. This should be quick to review for someone familiar with Helm and the project's conventions.
🧪 Relevant tests	No
🔍 Possible issues	Possible Bug: While initializing the script entries with empty strings solves the iteration issue in Helm, it might introduce unexpected behavior if other parts of the chart or dependent charts rely on these values being explicitly nil or not set. It's important to verify that this change does not affect other templates or logic expecting nil values.
🔒 Security concerns	No

---

✨ Review tool usage guide:

---

Overview:
The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

• When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

• With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

[qodo-merge-pro]

PR Code Suggestions

Category	Suggestions
Best practice	Use null instead of empty strings for uninitialized script content. Instead of setting empty strings for script content, consider using null or omitting the key entirely if the script content is not required. This approach is more idiomatic in YAML and can help avoid confusion with actual empty string values. charts/selenium-grid/values.yaml [116-117] -nodePreStop.sh: "" -nodeProbe.sh: "" +nodePreStop.sh: null +nodeProbe.sh: null
	Ensure consistency in handling script entries without content. For consistency and clarity, consider using the same approach for all extraScripts entries across different config maps. If you decide to use null or omit the key for scripts without content, apply this uniformly. charts/selenium-grid/values.yaml [157] -upload.sh: "" +upload.sh: null
Maintainability	Remove empty string assignments for commented-out script entries. For the commented-out script entries, it's better to remove the empty string assignment. If these scripts are optional or for documentation purposes, leaving them without an assignment or using null makes the intention clearer and the file cleaner. charts/selenium-grid/values.yaml [139-140] -# video.sh: "" -# video_graphQLQuery.sh: "" +# video.sh: +# video_graphQLQuery.sh:

---

✨ Improve tool usage guide:

---

Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

• When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:

/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...

• With a configuration file, use the following template:

[pr_code_suggestions]
some_config1=...
some_config2=...

See the improve usage page for a comprehensive guide on using this tool.

[VietND96]

May I know the Helm version that you could reproduce this issue?

[maxmanuylov]

Sure, it's
version.BuildInfo{Version:"v3.14.2", GitCommit:"c309b6f0ff63856811846ce18f3bdc93d2b4d54b", GitTreeState:"clean", GoVersion:"go1.22.0"}

[VietND96]

Yes, it is the same version that is used in CI tests. There is a template test rely on helm template and YAML assertion. It also included the case where chart selenium-grid is imported as dependency and assert YAML output, but it passed
https://github.com/SeleniumHQ/docker-selenium/blob/trunk/tests/charts/bootstrap.sh
https://github.com/SeleniumHQ/docker-selenium/blob/trunk/tests/charts/templates/test.py
https://github.com/SeleniumHQ/docker-selenium/blob/trunk/tests/charts/umbrella-charts/Chart.yaml
So, I also try to understand any missing there.

[VietND96]

Thank you, @maxmanuylov!
If you have time and interest, can you look into the template tests to see if it is effective to catch such a bug?