Make Helm chart portable by removing hardcoded private image registry#167
Make Helm chart portable by removing hardcoded private image registry#167saros-dev wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: saros-dev The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @saros-dev! |
|
Hi @saros-dev. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Thanks for the review requests. I have completed the CLA authorization. Please let me know if any additional changes are needed. Thank you! |
d54df0c to
31dda86
Compare
|
/ok-to-test |
ffromani
left a comment
There was a problem hiding this comment.
Thanks for your PR!
Improving devex/ux is surely something we want to do, and indeed we can have better defaults. Likely, we should default to the previous released version by default and document how to point to a custom-built development version.
That said, unfortunately it is not obvious to me how the PR solves the problem you are describing. Perhaps you could maybe elaborate a bit?
| repository: dra-driver-cpu # @schema required:true | ||
| # -- Image tag; defaults to `.Chart.AppVersion` when empty, which is set to the release tag at package time | ||
| tag: "" | ||
| tag: "latest" |
There was a problem hiding this comment.
I think this should be left "" because this way helm can fall back to .Chart.AppVersion. Then yes, nowadays .Chart.Appversion is latest, but I think we want to keep the indirection
| image: | ||
| # -- Container image repository | ||
| repository: us-central1-docker.pkg.dev/k8s-staging-images/dra-driver-cpu/dra-driver-cpu # @schema required:true | ||
| repository: dra-driver-cpu # @schema required:true |
There was a problem hiding this comment.
I think --set image.repository should already allow to override this value. Is that the case? is there another issue which is eluding us?
|
Thanks for the review. The issue I encountered was during a clean Helm installation using the chart defaults. The resulting DaemonSet attempted to pull the image from:
and consistently failed with My initial assumption was that the chart defaults should point to an image that is directly consumable by users evaluating the project or running it in local development environments. Based on that assumption, I proposed changing the default repository to avoid the dependency on the staging registry. That said, I understand your point that Perhaps a better approach would be to document the expected workflow for local development and clarify which image repository users should override to when deploying the chart outside of the Kubernetes release infrastructure. I am happy to adjust the PR direction if documentation or another approach would be preferred. |
Problem
The Helm chart currently uses a hardcoded image from a private GCP Artifact Registry:
us-central1-docker.pkg.dev/k8s-staging-images/dra-driver-cpu/dra-driver-cpu:latest
This causes
ImagePullBackOfffor contributors who do not have access to the registry.Impact
Changes in this PR
values.yamlimagePullSecretsHow to test
helm template ./deployment/helm/dra-driver-cpu
Notes
This change improves chart portability without changing default behavior for existing users.
This change is backward compatible and does not modify default behavior.
Happy to adjust this if maintainers prefer a different approach.