Copilot commented on code in PR #64164:
URL: https://github.com/apache/airflow/pull/64164#discussion_r3025331140
##########
chart/values.yaml:
##########
@@ -3508,6 +3508,7 @@ databaseCleanup:
# When set, overwrite the default k8s number of successful and failed
CronJob executions that are saved.
failedJobsHistoryLimit: 1
successfulJobsHistoryLimit: 1
Review Comment:
The newly added `ttlSecondsAfterFinished` value lacks an explanatory comment
in values.yaml. Nearby fields in this section are documented, and users
commonly rely on values.yaml comments. Add a brief description (and note the
Kubernetes version requirement if applicable) to keep the chart’s values
documentation consistent.
```suggestion
successfulJobsHistoryLimit: 1
# Time to live (in seconds) for Jobs created by this CronJob after they
finish.
# Requires Kubernetes with the TTL controller for finished resources
enabled (Kubernetes 1.21+ recommended).
```
##########
chart/templates/database-cleanup/database-cleanup-cronjob.yaml:
##########
@@ -56,6 +56,9 @@ spec:
jobTemplate:
spec:
backoffLimit: 1
+ {{- if ne .Values.databaseCleanup.ttlSecondsAfterFinished nil }}
Review Comment:
The ttlSecondsAfterFinished conditional is inconsistent with the other Job
templates in this chart (e.g. create-user-job.yaml and
migrate-database-job.yaml use `not (kindIs "invalid" ...)`). Using `ne ... nil`
can behave differently for an unset value (`<no value>`) and is less robust if
schema validation is skipped. Consider switching to the same `kindIs "invalid"`
check used elsewhere for ttlSecondsAfterFinished so missing/null values won’t
render and won’t risk template comparison errors.
```suggestion
{{- if not (kindIs "invalid"
.Values.databaseCleanup.ttlSecondsAfterFinished) }}
```
##########
helm-tests/tests/helm_tests/airflow_aux/test_database_cleanup.py:
##########
@@ -508,3 +508,28 @@ def test_overridden_automount_service_account_token(self):
show_only=["templates/database-cleanup/database-cleanup-serviceaccount.yaml"],
)
assert jmespath.search("automountServiceAccountToken", docs[0]) is
False
+
+ @pytest.mark.parametrize(
+ ("ttl_value", "expected_rendered"),
+ [
+ (300, 300),
+ (0, 0),
+ (None, None),
+ ],
+ )
+ def test_ttl_seconds_after_finished_rendering(self, ttl_value,
expected_rendered):
Review Comment:
This test is placed under `TestDatabaseCleanupServiceAccount`, but it
renders/asserts against the CronJob template (database-cleanup-cronjob.yaml)
rather than the ServiceAccount template. Please move it into the main
`TestDatabaseCleanup` class (or a dedicated CronJob-focused class) so the test
grouping matches what’s being exercised.
--
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]
