jedcunningham commented on code in PR #24784:
URL: https://github.com/apache/airflow/pull/24784#discussion_r938085718
##########
tests/charts/test_scheduler.py:
##########
@@ -115,6 +115,39 @@ def
test_should_add_extra_volume_and_extra_volume_mount(self):
"spec.template.spec.containers[0].volumeMounts[*].name", docs[0]
)
+ def test_should_add_extraEnvs(self):
+ docs = render_chart(
+ values={
+ "executor": "CeleryExecutor",
+ "scheduler": {
+ "env": [{"name": "TEST_ENV_1", "value": "test_env_1"}],
+ },
+ },
+ show_only=["templates/scheduler/scheduler-deployment.yaml"],
+ )
+
+ assert {'name': 'TEST_ENV_1', 'value': 'test_env_1'} in
jmespath.search(
+ "spec.template.spec.containers[0].env", docs[0]
+ )
+
+ def test_should_add_extraEnvs_to_wait_for_migration_container(self):
+ docs = render_chart(
+ values={
+ "executor": "CeleryExecutor",
+ "scheduler": {
+ "waitForMigrations": {
+ "enabled": True,
Review Comment:
```suggestion
```
Also the default.
##########
tests/charts/test_pod_template_file.py:
##########
@@ -639,6 +639,16 @@ def test_should_add_pod_labels(self):
"tier": "airflow",
} == jmespath.search("metadata.labels", docs[0])
+ def test_should_add_extraEnvs(self):
+ docs = render_chart(
+ values={"env": [{"name": "TEST_ENV_1", "value": "test_env_1"}]},
Review Comment:
```suggestion
values={"worker": {"env": [{"name": "TEST_ENV_1", "value":
"test_env_1"}]}},
```
##########
tests/charts/test_webserver.py:
##########
@@ -169,6 +169,39 @@ def test_should_add_extra_containers(self):
"image": "test-registry/test-repo:test-tag",
} == jmespath.search("spec.template.spec.containers[-1]", docs[0])
+ def test_should_add_extraEnvs(self):
+ docs = render_chart(
+ values={
+ "executor": "CeleryExecutor",
Review Comment:
```suggestion
```
##########
chart/templates/dag-processor/dag-processor-deployment.yaml:
##########
@@ -134,6 +134,9 @@ spec:
{{- if .Values.dagProcessor.extraInitContainers }}
{{- toYaml .Values.dagProcessor.extraInitContainers | nindent 8 }}
{{- end }}
+{{- if .Values.dagProcessor.waitForMigrations.env }}
+{{ tpl (toYaml .Values.dagProcessor.waitForMigrations.env) $ | indent 12 }}
+{{- end }}
Review Comment:
This isn't in the right spot. Needs to move up to ensure it's in the envs.
##########
chart/values.schema.json:
##########
@@ -2619,6 +2698,14 @@
"description": "Specify if you want to use the default
Helm Hook annotations",
"type": "boolean",
"default": true
+ },
+ "env": {
+ "description": "Add additional env vars to job.",
Review Comment:
```suggestion
"description": "Add additional env vars to the create
user job pod.",
```
##########
tests/charts/test_worker.py:
##########
@@ -112,6 +112,36 @@ def
test_should_add_extra_volume_and_extra_volume_mount(self):
"spec.template.spec.containers[0].volumeMounts[0].name", docs[0]
)
+ def test_should_add_extraEnvs(self):
+ docs = render_chart(
+ values={
+ "executor": "CeleryExecutor",
Review Comment:
```suggestion
```
##########
tests/charts/test_worker.py:
##########
@@ -112,6 +112,36 @@ def
test_should_add_extra_volume_and_extra_volume_mount(self):
"spec.template.spec.containers[0].volumeMounts[0].name", docs[0]
)
+ def test_should_add_extraEnvs(self):
+ docs = render_chart(
+ values={
+ "executor": "CeleryExecutor",
+ "workers": {
+ "env": [{"name": "TEST_ENV_1", "value": "test_env_1"}],
+ },
+ },
+ show_only=["templates/workers/worker-deployment.yaml"],
+ )
+
+ assert {'name': 'TEST_ENV_1', 'value': 'test_env_1'} in
jmespath.search(
+ "spec.template.spec.containers[0].env", docs[0]
+ )
+
+ def test_should_add_extraEnvs_to_wait_for_migration_container(self):
+ docs = render_chart(
+ values={
+ "executor": "CeleryExecutor",
Review Comment:
```suggestion
```
##########
tests/charts/test_triggerer.py:
##########
@@ -131,6 +131,39 @@ def
test_should_add_extra_volume_and_extra_volume_mount(self):
"spec.template.spec.containers[0].volumeMounts[0].name", docs[0]
)
+ def test_should_add_extraEnvs(self):
+ docs = render_chart(
+ values={
+ "triggerer": {
+ "enabled": True,
Review Comment:
```suggestion
```
##########
tests/charts/test_scheduler.py:
##########
@@ -115,6 +115,39 @@ def
test_should_add_extra_volume_and_extra_volume_mount(self):
"spec.template.spec.containers[0].volumeMounts[*].name", docs[0]
)
+ def test_should_add_extraEnvs(self):
+ docs = render_chart(
+ values={
+ "executor": "CeleryExecutor",
Review Comment:
```suggestion
```
This is the default.
##########
tests/charts/test_webserver.py:
##########
@@ -169,6 +169,39 @@ def test_should_add_extra_containers(self):
"image": "test-registry/test-repo:test-tag",
} == jmespath.search("spec.template.spec.containers[-1]", docs[0])
+ def test_should_add_extraEnvs(self):
+ docs = render_chart(
+ values={
+ "executor": "CeleryExecutor",
+ "webserver": {
+ "env": [{"name": "TEST_ENV_1", "value": "test_env_1"}],
+ },
+ },
+ show_only=["templates/webserver/webserver-deployment.yaml"],
+ )
+
+ assert {'name': 'TEST_ENV_1', 'value': 'test_env_1'} in
jmespath.search(
+ "spec.template.spec.containers[0].env", docs[0]
+ )
+
+ def test_should_add_extraEnvs_to_wait_for_migration_container(self):
+ docs = render_chart(
+ values={
+ "executor": "CeleryExecutor",
+ "webserver": {
+ "waitForMigrations": {
+ "enabled": True,
Review Comment:
```suggestion
"webserver": {
"waitForMigrations": {
```
##########
tests/charts/test_scheduler.py:
##########
@@ -115,6 +115,39 @@ def
test_should_add_extra_volume_and_extra_volume_mount(self):
"spec.template.spec.containers[0].volumeMounts[*].name", docs[0]
)
+ def test_should_add_extraEnvs(self):
+ docs = render_chart(
+ values={
+ "executor": "CeleryExecutor",
+ "scheduler": {
+ "env": [{"name": "TEST_ENV_1", "value": "test_env_1"}],
+ },
+ },
+ show_only=["templates/scheduler/scheduler-deployment.yaml"],
+ )
+
+ assert {'name': 'TEST_ENV_1', 'value': 'test_env_1'} in
jmespath.search(
+ "spec.template.spec.containers[0].env", docs[0]
+ )
+
+ def test_should_add_extraEnvs_to_wait_for_migration_container(self):
+ docs = render_chart(
+ values={
+ "executor": "CeleryExecutor",
Review Comment:
```suggestion
```
##########
chart/values.yaml:
##########
@@ -597,6 +597,12 @@ workers:
# cpu: 100m
# memory: 128Mi
+ waitForMigrations:
+ # To add env vars specific to wait-for-migrations init container
Review Comment:
```suggestion
```
##########
chart/templates/workers/worker-deployment.yaml:
##########
@@ -223,6 +226,9 @@ spec:
{{- if and (.Values.dags.gitSync.enabled) (not
.Values.dags.persistence.enabled) }}
{{- include "git_sync_container" . | indent 8 }}
{{- end }}
+{{- if .Values.workers.env }}
+{{ tpl (toYaml .Values.workers.env) $ | indent 12 }}
+{{- end }}
Review Comment:
Same here, needs to be moved to the right spot.
##########
chart/files/pod-template-file.kubernetes-helm-yaml:
##########
@@ -58,6 +58,9 @@ spec:
value: LocalExecutor
{{- include "standard_airflow_environment" . | indent 6}}
{{- include "custom_airflow_environment" . | indent 6 }}
+{{- if .Values.workers.env }}
+{{ tpl (toYaml .Values.workers.env) $ | indent 12 }}
Review Comment:
```suggestion
{{ tpl (toYaml .Values.workers.env) $ | indent 8 }}
```
I think this is the right indent?
##########
tests/charts/test_triggerer.py:
##########
@@ -131,6 +131,39 @@ def
test_should_add_extra_volume_and_extra_volume_mount(self):
"spec.template.spec.containers[0].volumeMounts[0].name", docs[0]
)
+ def test_should_add_extraEnvs(self):
+ docs = render_chart(
+ values={
+ "triggerer": {
+ "enabled": True,
+ "env": [{"name": "TEST_ENV_1", "value": "test_env_1"}],
+ }
+ },
+ show_only=["templates/triggerer/triggerer-deployment.yaml"],
+ )
+
+ assert {'name': 'TEST_ENV_1', 'value': 'test_env_1'} in
jmespath.search(
+ "spec.template.spec.containers[0].env", docs[0]
+ )
+
+ def test_should_add_extraEnvs_to_wait_for_migration_container(self):
+ docs = render_chart(
+ values={
+ "triggerer": {
+ "enabled": True,
+ "waitForMigrations": {
+ "enabled": True,
Review Comment:
```suggestion
"waitForMigrations": {
```
--
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]
