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

Add additionalVolumes and additionalVolumeMounts to coordinator and worker #129

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

koh-satoh-wpg
Copy link
Member

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

Copy link

cla-bot bot commented Jan 30, 2024

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
Copy link
Member Author

Currently waiting for my signed CLA to be accepted.

@BrandenRice
Copy link

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
Copy link
Member

mosabua commented Feb 2, 2024

@cla-bot check

Copy link

cla-bot bot commented Feb 2, 2024

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

Copy link

cla-bot bot commented Feb 2, 2024

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

@koh-satoh-wpg koh-satoh-wpg marked this pull request as ready for review February 6, 2024 01:05
@koh-satoh-wpg
Copy link
Member Author

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
Copy link
Member Author

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Feb 14, 2024
Copy link

cla-bot bot commented Feb 14, 2024

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

@koh-satoh-wpg
Copy link
Member Author

@mosabua Is this something you can review? Thanks!

@BrandenRice
Copy link

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.

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Looks good. Please add the docs so it gets rendered out by Frigate into the README.

charts/trino/values.yaml Outdated Show resolved Hide resolved
charts/trino/values.yaml Outdated Show resolved Hide resolved
charts/trino/values.yaml Outdated Show resolved Hide resolved
charts/trino/values.yaml Outdated Show resolved Hide resolved
@mosabua
Copy link
Member

mosabua commented Feb 27, 2024

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.

Copy link
Member Author

@koh-satoh-wpg koh-satoh-wpg left a comment

Choose a reason for hiding this comment

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

Add comments on the new value fields

@koh-satoh-wpg
Copy link
Member Author

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

@mosabua
Copy link
Member

mosabua commented Feb 27, 2024

Please remove the myvalues file and squash commits

@koh-satoh-wpg
Copy link
Member Author

oh my 🤦 done & done. thx!

@mosabua
Copy link
Member

mosabua commented Feb 28, 2024

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.

@mosabua mosabua merged commit 6b68924 into trinodb:main Feb 28, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants