Added custom environment variables to statsd#28034
Added custom environment variables to statsd#28034RagingPuppies wants to merge 7 commits intoapache:mainfrom RagingPuppies:statsd_custom_env
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
|
jedcunningham
left a comment
There was a problem hiding this comment.
The chart does have unit tests as well!
https://github.com/apache/airflow/blob/main/TESTING.rst#helm-unit-tests
https://github.com/apache/airflow/tree/main/tests/charts
| subPath: mappings.yml | ||
| {{- if .Values.statsd.env}} | ||
| env: | ||
| {{- include "container_extra_envs" (list . .Values.statsd.env) | indent 10 }} |
There was a problem hiding this comment.
Since this isn't an Airflow container, we don't need to use this helper here.
There was a problem hiding this comment.
@jedcunningham as Flower is not an airflow container as well, but it did use the same code, don't you think it should be aligned?
There was a problem hiding this comment.
The difference is flower is started via the Airflow cli, and this run in a completely separate image that doesn't even have Airflow.
Co-authored-by: Jed Cunningham <66968678+jedcunningham@users.noreply.github.com>
|
Are you still working on it @RagingPuppies ? If so - it needs rebase/conflict resolving |
|
yes, @potiuk didn't have the time, pushed a new commit to fix @jedcunningham rejects. |
|
Some conflicts still need to be solved @RagingPuppies now |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
Hi,
This is not related to any issue, all airflow components have the option to add custom env to their docker container except statsd, I've just added the same functionality and tested the chart in debug mode. (as I don't think the unit tests have any relation to the helm charts.)