Copilot commented on code in PR #64032:
URL: https://github.com/apache/airflow/pull/64032#discussion_r3066475298
##########
chart/templates/_helpers.yaml:
##########
@@ -1138,7 +1138,7 @@ Usage:
{{- if gt (len $nested) 0 -}}
{{- $_ := set $newValues $key $nested -}}
{{- end -}}
- {{- else if not (eq $val nil) -}}
+ {{- else if $val -}}
Review Comment:
Switching from `not (eq $val nil)` to `if $val` changes semantics beyond
fixing the slice/nil comparison: it will now drop valid falsy scalar values
(e.g., `false`, `0`, `\"\"`) in addition to empty slices/maps. If this helper
is intended to filter only nil/empties (or specifically avoid the slice-`nil`
comparison), consider using a condition that safely handles slices without
removing explicit falsy scalars—for example by using `empty`/`len` for
collections while still preserving boolean `false` (and other scalars) when
explicitly set.
```suggestion
{{- else if or (kindIs "slice" $val) (kindIs "array" $val) -}}
{{- if gt (len $val) 0 -}}
{{- $_ := set $newValues $key $val -}}
{{- end -}}
{{- else if not (kindIs "invalid" $val) -}}
```
##########
helm-tests/tests/helm_tests/airflow_core/test_worker_sets.py:
##########
@@ -37,6 +37,22 @@ def test_enable_default_worker_set(self, enable_default,
objects_number):
assert objects_number == len(docs)
+ def test_create_worker_with_sets_empty(self):
Review Comment:
The test name is a bit ambiguous about the expected behavior. Consider
renaming it to reflect the assertion more explicitly (e.g., that an empty
`sets` list results in only the default worker being rendered), which makes the
regression intent clearer when scanning test output.
```suggestion
def test_empty_worker_sets_renders_only_default_worker(self):
```
##########
helm-tests/tests/helm_tests/airflow_core/test_worker_sets.py:
##########
@@ -37,6 +37,22 @@ def test_enable_default_worker_set(self, enable_default,
objects_number):
assert objects_number == len(docs)
+ def test_create_worker_with_sets_empty(self):
+ docs = render_chart(
+ name="test",
+ values={
+ "workers": {
+ "celery": {
+ "enableDefault": True,
+ "sets": [],
+ },
+ },
+ },
+ show_only=["templates/workers/worker-deployment.yaml"],
+ )
+
+ assert jmespath.search("[*].metadata.name", docs) == ['test-worker']
Review Comment:
The test name is a bit ambiguous about the expected behavior. Consider
renaming it to reflect the assertion more explicitly (e.g., that an empty
`sets` list results in only the default worker being rendered), which makes the
regression intent clearer when scanning test output.
##########
chart/templates/_helpers.yaml:
##########
@@ -1138,7 +1138,7 @@ Usage:
{{- if gt (len $nested) 0 -}}
{{- $_ := set $newValues $key $nested -}}
{{- end -}}
- {{- else if not (eq $val nil) -}}
+ {{- else if $val -}}
Review Comment:
This behavior change (truthiness-based filtering) is only covered for the
`sets: []` case via the new test, but it can also affect explicitly configured
falsy values (e.g., settings intentionally set to `false`). Please add a
regression test that sets a known boolean value to `false` in chart values and
asserts it remains effective in the rendered manifests (or clarify in tests
that falsy scalars are expected to be dropped).
```suggestion
{{- else if not (kindIs "invalid" $val) -}}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
