search

trinodb / charts

Apache License 2.0
133 stars 149 forks source link

Added more options for specifying the container image #138

Closed zltyfsh closed 3 months ago

zltyfsh commented 4 months ago

We are using both a custom image registry (for caching/mirroring) and pinning the images using the image digest (instead of tag).

This pull request adds support for both custom registry and using a digest, and is backwards compatible with the current model. The actual code are inspired of the bitnami/common helm-chart.

The chart README.md file is not updated, as I assume it is regenerated in the CI/CD pipeline.

cla-bot[bot] commented 4 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

mosabua commented 4 months ago

@cla-bot check

cla-bot[bot] commented 4 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 4 months ago

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

cla-bot[bot] commented 4 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

zltyfsh commented 3 months ago

@cla-bot check

cla-bot[bot] commented 3 months ago

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

huw0 commented 3 months ago

Being able to deploy with an image digest is a really key feature. However it'd be handy if in the trino.image definition this could also support overriding everything as a single value?

My build system outputs registry + repository + digest as a single value and I'd like to avoid having a step to split these.

zltyfsh commented 3 months ago

@huw0 I don't think that is achievable without adding a new (5th) key to image, which makes the interface a bit overloaded (at least in my eyes).

In theory the repository key could be used as the sole image reference if all the other keys (registry, tag and digest) were empty. But that is not backwards compatible with the current interface, because when the tag key is empty .Chart.AppVersion is used as default tag (if digest also is empty).

mosabua commented 3 months ago

Did you forget to push @zltyfsh ?

zltyfsh commented 3 months ago

After some thinking, I've come up with a solution that supports the use-case suggested by @huw0 in https://github.com/trinodb/charts/pull/138/commits/cbc641efaefa26cec969dcb04724de46799c0d4d

huw0 commented 3 months ago

After some thinking, I've come up with a solution that supports the use-case suggested by @huw0 in cbc641e

Thanks, this is perfect for my use case