jedcunningham commented on code in PR #32349:
URL: https://github.com/apache/airflow/pull/32349#discussion_r1278066437
##########
chart/files/pod-template-file.kubernetes-helm-yaml:
##########
@@ -23,6 +23,7 @@
{{- $topologySpreadConstraints := or .Values.workers.topologySpreadConstraints
.Values.topologySpreadConstraints }}
{{- $securityContext := include "airflowPodSecurityContext" (list .
.Values.workers) }}
{{- $containerSecurityContext := include "containerSecurityContext" (list .
.Values.workers) }}
+{{- $containerLifecycleHooks := include "containerLifecycleHooks" (list .
.Values.workers) }}
Review Comment:
I'm not sure we need a named template for this. We can do this:
```suggestion
{{- $containerLifecycleHooks := or .Values.workers.containerLifecycleHooks
.Values.ContainerLifecycleHooks }}
```
##########
chart/files/pod-template-file.kubernetes-helm-yaml:
##########
@@ -64,6 +65,9 @@ spec:
image: {{ template "pod_template_image" . }}
imagePullPolicy: {{ .Values.images.pod_template.pullPolicy }}
securityContext: {{ $containerSecurityContext | nindent 8 }}
+ {{- if $containerLifecycleHooks }}
+ lifecycle: {{- $containerLifecycleHooks | nindent 8 }}
Review Comment:
And to the tpl here:
```suggestion
lifecycle: {{- tpl $containerLifecycleHooks . | nindent 8 }}
```
##########
chart/values.yaml:
##########
@@ -43,6 +43,11 @@ securityContexts:
pod: {}
containers: {}
+# Default container level lifecycle hooks for every container
+# See:
+# https://kubernetes.io/docs/concepts/containers/container-lifecycle-hooks/
Review Comment:
I don't think we need to link folks to this everywhere. Easy enough to
search for it.
```suggestion
```
##########
helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -408,3 +408,39 @@ def test_priority_class_name(self):
priority = doc["spec"]["template"]["spec"]["priorityClassName"]
assert priority == f"low-priority-{component}"
+
+ # Test containerLifecycleHooks for main containers
+ def test_global_container_lifecycle_webhooks(self):
+ post_start_value = {"exec": {"command": ["bash", "-c", "echo
postStart"]}}
Review Comment:
Can you add something that exercises templating?
##########
helm_tests/airflow_aux/test_airflow_common.py:
##########
@@ -408,3 +408,39 @@ def test_priority_class_name(self):
priority = doc["spec"]["template"]["spec"]["priorityClassName"]
assert priority == f"low-priority-{component}"
+
+ # Test containerLifecycleHooks for main containers
+ def test_global_container_lifecycle_webhooks(self):
+ post_start_value = {"exec": {"command": ["bash", "-c", "echo
postStart"]}}
+ pre_stop_value = {"exec": {"command": ["bash", "-c", "echo preStop"]}}
+ docs = render_chart(
+ values={
+ "containerLifecycleHooks": {
+ "postStart": post_start_value,
+ "preStop": pre_stop_value,
+ },
+ },
+ show_only=[
+ "templates/flower/flower-deployment.yaml",
+ "templates/scheduler/scheduler-deployment.yaml",
+ "templates/webserver/webserver-deployment.yaml",
+ "templates/workers/worker-deployment.yaml",
+ "templates/jobs/create-user-job.yaml",
+ "templates/jobs/migrate-database-job.yaml",
+ "templates/triggerer/triggerer-deployment.yaml",
+ "templates/statsd/statsd-deployment.yaml",
+ "templates/redis/redis-statefulset.yaml",
+ ],
+ )
+
+ for index in range(len(docs) - 2):
Review Comment:
Can you refactor this to use list slices instead? e.g like
[this](https://github.com/apache/airflow/blob/0fbb05751acf6061f9f6fa83ee53816d568055d0/helm_tests/security/test_security_context.py#L247).
--
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]
