Copilot commented on code in PR #64164:
URL: https://github.com/apache/airflow/pull/64164#discussion_r3066479739
##########
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:
For consistency with the other chart job templates, consider using the same
presence check pattern for ttlSecondsAfterFinished (e.g. `{{- if not (kindIs
"invalid" ...) }}`) instead of `ne ... nil`. The create-user and
migrate-database job templates use `kindIs "invalid"` for this field, which
also covers "unset" values cleanly.
##########
chart/values.schema.json:
##########
@@ -10840,6 +10840,14 @@
],
"default": 1,
"x-docsSection": "Kubernetes"
+ },
+ "ttlSecondsAfterFinished": {
+ "description": "The number of seconds after a finished Job
is eligible to be automatically deleted.",
+ "type": [
+ "integer",
+ "null"
+ ],
+ "default": null
Review Comment:
In the databaseCleanup schema, adjacent Kubernetes-related properties
(failedJobsHistoryLimit/successfulJobsHistoryLimit) include `x-docsSection:
"Kubernetes"`, but ttlSecondsAfterFinished does not. Consider adding the same
x-docsSection so docs/grouping remain consistent within this object.
##########
helm-tests/tests/helm_tests/airflow_aux/test_database_cleanup.py:
##########
@@ -507,3 +507,29 @@ 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
+
+ def test_ttl_seconds_after_finished_default_behavior(self):
+ values = {"databaseCleanup": {"enabled": True}}
+ docs = render_chart(
+ values=values,
+
show_only=["templates/database-cleanup/database-cleanup-cronjob.yaml"],
+ )
+
+ assert "ttlSecondsAfterFinished" not in
docs[0]["spec"]["jobTemplate"]["spec"]
+
+ @pytest.mark.parametrize(
+ ("ttl_value", "expected_rendered"),
+ [
+ (300, 300),
+ (0, 0),
+ ],
+ )
+ def test_ttl_seconds_after_finished_rendering(self, ttl_value,
expected_rendered):
+ values = {"databaseCleanup": {"enabled": True,
"ttlSecondsAfterFinished": ttl_value}}
+ docs = render_chart(
+ values=values,
+
show_only=["templates/database-cleanup/database-cleanup-cronjob.yaml"],
+ )
+
+ actual_ttl =
jmespath.search("spec.jobTemplate.spec.ttlSecondsAfterFinished", docs[0])
+ assert actual_ttl == expected_rendered
Review Comment:
These ttlSecondsAfterFinished assertions are placed under
TestDatabaseCleanupServiceAccount, but they render/assert against the CronJob
template. To keep the test suite organized and avoid confusion, move these
tests into the TestDatabaseCleanup class (which already covers
database-cleanup-cronjob.yaml).
--
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]
