trinodb / charts

Apache License 2.0
151 stars 173 forks source link

Add additionalVolumes and additionalVolumeMounts to coordinator and worker #129

Closed koh-satoh-wpg closed 8 months ago

koh-satoh-wpg commented 9 months ago

Purpose

This change will allow the chart to take in the additional volume configurations for the coordinator and the worker.

For my use case, this is desired to authenticate the Trino Pods to a GCP project using workload identity federation. ref) https://cloud.google.com/iam/docs/workload-identity-federation-with-kubernetes#deploy

Others seem to be interested in mounting emptyDirs etc (see Related PRs).

Related PRs

cla-bot[bot] commented 9 months ago

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

koh-satoh-wpg commented 9 months ago

Currently waiting for my signed CLA to be accepted.

BrandenRice commented 9 months ago

I would love to see this PR approved! I am trying to inject a sidecar into my Trino pods (via sidecarContainers in the Helm chart), but I have no way to specify a volume containing configuration properties for my sidecar 😔. These changes to the Helm chart are just what I'm looking for, +1 on this.

mosabua commented 9 months ago

@cla-bot check

cla-bot[bot] commented 9 months ago

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot[bot] commented 9 months ago

The cla-bot has been summoned, and re-checked this pull request!

koh-satoh-wpg commented 9 months ago

Still waiting for the CLA acceptance but making this ready for review as it seems the PR review process is done in parallel.

koh-satoh-wpg commented 9 months ago

@cla-bot check

cla-bot[bot] commented 9 months ago

The cla-bot has been summoned, and re-checked this pull request!

koh-satoh-wpg commented 9 months ago

@mosabua Is this something you can review? Thanks!

BrandenRice commented 8 months ago

Congrats on finally getting your CLA signature accepted, @koh-satoh-wpg. I'd also like to +1 this for review, as this is a feature that we really need on the Helm chart, and don't want to fork the official one.

mosabua commented 8 months ago

And btw.. yes .. we should add this kind of doc info for all the other values as well over time. I just did not have a chance to get that going.

koh-satoh-wpg commented 8 months ago

Thanks @mosabua for the suggestions. Yeah I agree adding comments does make sense. Applied your suggestions.

mosabua commented 8 months ago

Please remove the myvalues file and squash commits

koh-satoh-wpg commented 8 months ago

oh my 🤦 done & done. thx!

mosabua commented 8 months ago

Any concerns @nineinchnick @losipiuk @radek-starburst or others? This one would be helpful for file system caching usage... I will look at some other PRs to merge maybe and tag a new release later this week.