allow env support for airflow loggroomer sidecar #1517
Conversation
| env: | ||
| {{- if .Values.dagProcessor.logGroomerSidecar.retentionDays }} | ||
| - name: AIRFLOW__LOG_RETENTION_DAYS | ||
| value: "{{ .Values.dagProcessor.logGroomerSidecar.retentionDays }}" | ||
| {{- end }} | ||
| - name: AIRFLOW_HOME | ||
| value: "{{ .Values.airflowHome }}" |
There was a problem hiding this comment.
This used to be that if retentionDays was not set, AIRFLOW_HOME would not be set. The new behavior always sets AIRFLOW_HOME. is that intentional? It seems to me like the new behavior would be correct, but it is a change from the old behavior. I think we should get an opinion from somebody on the airflow team.
There was a problem hiding this comment.
@dstandish When we submit this as a PR to apache/airflow, will they ask for any changes, or do you think this will be fine as it is?
There was a problem hiding this comment.
PR is here btw: apache#46003
There was a problem hiding this comment.
There is also this pr to add env to these containers: apache#45483
Different approaches, but I think this one is better.
Not sure why this and the OSS one are different though. I agree - not setting home seems like a bug.
There was a problem hiding this comment.
Thanks! I just want to make sure we're not diverging from upstream airflow here.
pgvishnuram
force-pushed
the
feat/add-envsupport-loggroomer
branch
from
92bfbdb to
3c13f46
Compare
|
@jedcunningham @dstandish why we raised seperate PR here because we want a custom cart on top of 1.13 oss chart and our timeline for 0.37 and patches are very short this will help us to quickly add patches to the custom branch and once oss merges the original PR we will rebase and publish the new charts on further releases |
Description
This PR adds support to allow configurable env support for airflow log groomer containers to inject custom env vars
Related Issues
Testing
QA should able to pass custom env vars for airflow log groomer container to override default log retention period
Merging
release custom chart on top of astronomer/airflow to astronomer/airflow-chart