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

Template extra volumes in helm chart #29357

Merged
merged 1 commit into from
Mar 13, 2023
Merged

Conversation

planoe
Copy link
Contributor

@planoe planoe commented Feb 3, 2023

This adds the possibility to define dynamic names (defined in custom helm template functions) for volumes and volumeMounts definitions such as secret names, config map names, mount paths etc.

I did not find a way to add tests in which I could define custom template functions, but I ran the chart tests successfully so it brings no regressions and I already use this in production, which works fine so far.

@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Feb 3, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Feb 3, 2023

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)
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: dev@airflow.apache.org
    Slack: https://s.apache.org/airflow-slack

@planoe
Copy link
Contributor Author

planoe commented Feb 10, 2023

@jedcunningham @dstandish what do you think about this feature ? I think it would be a valuable addition to make the chart more flexible. Many customers have different naming patterns in their K8S environment.

@potiuk
Copy link
Member

potiuk commented Feb 20, 2023

I think this one needs documentation explaining usage of this feature. Otherwise it won't be discoverable.

@planoe
Copy link
Contributor Author

planoe commented Feb 21, 2023

I think this one needs documentation explaining usage of this feature. Otherwise it won't be discoverable.

Fair point. I suggest to add a comment/example above the impacted fields in values.yaml (Not sure how many people actually read/use the schema.json file, I can image most of airflow admins check values.yaml first). For instance:

# Mount additional volumes into webserver. It can be templated like in the following example:
#   extraVolumes:
#     - name: my-templated-extra-volume
#       secret:
#          secretName: '{{ include "my_secret_template" . }}'
#          defaultMode: 0640
#          optional: true
#
#   extraVolumeMounts:
#     - name: my-templated-extra-volume
#       mountPath: "{{ .Values.my_custom_path }}"
#       readOnly: true

Would it be acceptable this way ? If so I will proceed with the changes

@potiuk
Copy link
Member

potiuk commented Feb 25, 2023

I think we also need a small chapter in Production Guide: https://airflow.apache.org/docs/helm-chart/stable/production-guide.html with short explanation of the use case and why you would like to do it.

@jedcunningham
Copy link
Member

jedcunningham commented Feb 25, 2023

My 2c, an example like @planoe suggested is enough. We can't put everything in the prod guide, it'd be way too long. We could consider documenting more advanced features, but we should do that as a separate effort I think.

@potiuk
Copy link
Member

potiuk commented Feb 25, 2023

My 2c, an example like @planoe suggested is enough. We can't put everything in the prod guide, it'd be way too long. We could consider documenting more advanced features, but we should do that as a separate effort I think.

Fine for me.

@planoe
Copy link
Contributor Author

planoe commented Mar 10, 2023

Sorry for the delay, I updated the comments. Hopefully it can be merged now

@potiuk
Copy link
Member

potiuk commented Mar 12, 2023

There ere merge conflicts now after reformatting of the chart templates . Need to be fixed

@planoe
Copy link
Contributor Author

planoe commented Mar 13, 2023

Thanks I fixed the merge conflicts.

@jedcunningham jedcunningham merged commit 198408e into apache:main Mar 13, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Mar 13, 2023

Awesome work, congrats on your first merged pull request!

@jedcunningham
Copy link
Member

Thanks @planoe! Congrats on your first commit 🎉

@planoe
Copy link
Contributor Author

planoe commented Mar 14, 2023

Thank you, hopefully more to come soon :)

nirroz93 added a commit to nirroz93/airflow that referenced this pull request Apr 20, 2023
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.

None yet

3 participants