Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

allow passing custom env to log groomer sidecar containers #46003

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pgvishnuram
Copy link
Contributor

@pgvishnuram pgvishnuram commented Jan 24, 2025

Issue Description:

Currently logGroomer sidecar container does not allow users to pass custom env vars

Changes:
This PR introduces env vars support for all the log groomer sidecars to allow overridable env vars to those components

Copy link
Contributor

@amoghrajesh amoghrajesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the need and like the idea, but do we need templating?

@pgvishnuram
Copy link
Contributor Author

I understand the need and like the idea, but do we need templating?

In every component of airflow helm template it was defined that way - so i included the same approach. I doesnt require we can directly do a toYaml directly as well

env:
{{- if .Values.dagProcessor.logGroomerSidecar.retentionDays }}
- name: AIRFLOW__LOG_RETENTION_DAYS
value: "{{ .Values.dagProcessor.logGroomerSidecar.retentionDays }}"
- name: AIRFLOW_HOME
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like discussed here, we should set AIRFLOW_HOME regardless.

@@ -12175,6 +12175,16 @@
"type": "integer",
"default": 15
},
"env": {
"description": "Add additional env vars to log groomer sidecar container.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"description": "Add additional env vars to log groomer sidecar container.",
"description": "Add additional env vars to log groomer sidecar container (templated).",

@@ -90,6 +90,19 @@ def test_log_groomer_collector_default_retention_days(self):
)
assert jmespath.search("spec.template.spec.containers[1].env[0].value", docs[0]) == "15"

def test_log_groomer_collector_custom_env(self):
env = {"name": "ENABLE_KUBE_MUTATION_TYPE", "value": "upsert"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be worth refactoring this test so it exercises the templating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants