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

Added more options for specifying the container image #138

Merged
merged 8 commits into from
Mar 14, 2024

Conversation

zltyfsh
Copy link
Contributor

@zltyfsh zltyfsh commented Feb 27, 2024

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.

Copy link

cla-bot bot commented Feb 27, 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 [email protected]. 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
Copy link
Member

mosabua commented Feb 29, 2024

@cla-bot check

Copy link

cla-bot bot commented Feb 29, 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 [email protected]. 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 29, 2024

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

Copy link

cla-bot bot commented Feb 29, 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 [email protected]. 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
Copy link
Contributor Author

zltyfsh commented Mar 4, 2024

@cla-bot check

@cla-bot cla-bot bot added the cla-signed label Mar 4, 2024
Copy link

cla-bot bot commented Mar 4, 2024

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

@huw0
Copy link
Member

huw0 commented Mar 12, 2024

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

zltyfsh commented Mar 12, 2024

@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).

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

@zltyfsh zltyfsh left a comment

Choose a reason for hiding this comment

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

Updated docs in value.yaml

@mosabua
Copy link
Member

mosabua commented Mar 14, 2024

Did you forget to push @zltyfsh ?

@zltyfsh
Copy link
Contributor Author

zltyfsh commented Mar 14, 2024

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

@zltyfsh zltyfsh changed the title Add support for image registry and digest Added more options for specifying the container image Mar 14, 2024
@zltyfsh zltyfsh requested a review from mosabua March 14, 2024 21:20
@huw0
Copy link
Member

huw0 commented Mar 14, 2024

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

charts/trino/values.yaml Outdated Show resolved Hide resolved
@zltyfsh zltyfsh requested a review from mosabua March 14, 2024 22:14
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 now.

@mosabua mosabua merged commit c7c652c into trinodb:main Mar 14, 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.

3 participants