ashb commented on a change in pull request #12303:
URL: https://github.com/apache/airflow/pull/12303#discussion_r522845915
##########
File path: airflow/models/renderedtifields.py
##########
@@ -102,7 +103,7 @@ def get_k8s_pod_yaml(cls, ti: TaskInstance, session:
Session = None) -> Optional
)
.one_or_none()
)
- return result.k8s_pod_yaml if result else None
+ return result.k8s_pod_yaml if result else ti.render_k8s_pod_yaml()
Review comment:
This shouldn't be done here -- it's breaking "encapsulation"/single
responsibility.
This should be in the webserver view I think.
##########
File path: tests/kubernetes/test_pod_generator.py
##########
@@ -473,6 +480,9 @@ def test_construct_pod_empty_executor_config(self,
mock_uuid):
worker_config.metadata.labels['app'] = 'myapp'
worker_config.metadata.name = 'pod_id-' + self.static_uuid.hex
worker_config.metadata.namespace = 'namespace'
+ worker_config.spec.containers[0].env.append(
+ k8s.V1EnvVar(name="AIRFLOW_IS_K8S_EXECUTOR_POD", value='True')
+ )
Review comment:
This is static -- it should be added in to
`pod_generator_base_with_secrets.yaml` instead of being added via code.
```suggestion
```
##########
File path: tests/kubernetes/test_pod_generator.py
##########
@@ -365,6 +365,7 @@ def test_reconcile_pods(self, mock_uuid):
image='',
name='name',
command=['/bin/command2.sh', 'arg2'],
+ env=[],
Review comment:
Is this change actually needed? It feels like it shouldn't be required,
and the code needs to deal with not env not being specified if it doesn't.
```suggestion
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
