-
Notifications
You must be signed in to change notification settings - Fork 5.1k
feat: add --image-repository support for Helm-based addons #22207
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
base: master
Are you sure you want to change the base?
feat: add --image-repository support for Helm-based addons #22207
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: PhantomInTheWire 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 @PhantomInTheWire! |
|
Hi @PhantomInTheWire. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with 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. |
|
Can one of the admins verify this patch? |
|
@PhantomInTheWire the issue you linked talks about doing one exmaple of helm based addon, we have not added one yet |
|
@medyagh i initially wanted to migrate an existing addon that but but figured adding this first would be needed for a full migration. i was under the impression that we want to migrate existing addons, is that not the case? i thought not being able to use --image-repository flag was the blocker, if not that what exactly would we need to cloase #21257 |
cc899a8 to
97503f6
Compare
|
@medyagh thanks for the feedback earlier! Just wanted to follow up and clarify intent: this PR only adds the plumbing needed for If the expectation is that this PR should also migrate at least one addon as an example, I’m happy to do that, just wanted to confirm the preferred direction before proceeding. Appreciate your guidance. |
97503f6 to
60feafd
Compare
|
I undrstood the intent of the PR I just meant that we currently dont have any Addon that uses helm, you could try with adding a helm-based addon, such as converting "Volcano" addon to be helm based and test your PR with ti |
| Namespace string | ||
| Values []string | ||
| ValueFiles []string | ||
| ImageSetKey string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PhantomInTheWire
why is this called ImageSetKey is that same Name used in helm flags? can you please put an example of using helm itself with image respostiry
also add comment to this field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
helm doesn't actually give us a clean way to do this and we have to use --set to manually set fields, i have added comments to explain this data structure better, also i was initially testing by migrating metrics server but working on volcano(on both dockerio and ghcrio) revealed that there were some bad assumptions i made regarding versions and how different registeries handle names, i have changed the pr up a little bit PTAL again
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PhantomInTheWire can you plz test this addon by moving volano addon to
https://github.com/volcano-sh/volcano to helm charts and then try the --image-repostiry flag with it
volcano is now on ghcr.io
volcano-sh/volcano#4846
276b4cb to
409de19
Compare
409de19 to
77d38bc
Compare
|
@medyagh sure i have tested and pushed the migration for volcano with image repository it pulls from where we ask ./out/minikube addons enable volcano --image-repository=ghcr.io/volcano-sh:latest
kubectl get pods -n volcano-system -o jsonpath='{range .items[*]}{.spec.containers[*].image}{"\n"}{end}'
# Expected: ghcr.io/volcano-sh/vc-*:latestby default it pulls in v1.13 from docker.io that was in the codebase earlier(cant pull this exact version from ghcr it seems it only has the latest version) ./out/minikube addons enable volcano
kubectl get pods -n volcano-system -o jsonpath='{range .items[*]}{.spec.containers[*].image}{"\n"}{end}'
# Expected: docker.io/volcanosh/vc-*:v1.13.1 |
thank you so much @PhantomInTheWire for the update, I am curious is "latest" needed there? what happens if user doesnt specificy ghcr.io/volcano-sh:latest also I am curious, instead of creating a new flag, why not use the existing one is there a good reason not to use --registries ? |
| if registries != "" { | ||
| viper.Set(config.AddonRegistries, registries) | ||
| } | ||
| if imageRepository != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of creating a new flag, why not use the existing one
is there a good reason not to use --registries ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there were some semantic and ux reasons as well but the major reason is:
missing org/tag support: --registries only handles the registry portion, but the Helm use case also needs to override the org path and optionally the tag (e.g., switching from docker.io/volcanosh:v1.13.0 to ghcr.io/volcano-sh:latest)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in helm world do they Have to specify the tag for the image respository or is that optional ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am mor ein favor of making things more simple and not add one more flag, I rather if we can just have one flag for both, (we can add an alias for a flag to match helm)
what is helms exact name and documetnation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in helm tag is optional and default is latest, but tags are widely used for getting specific version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Helm doesn't have a direct equivalent flag, it uses --set flag with chart-specific keys.
|
thankyou so much for the quick review @medyagh
it defaults to latest if nothing is specified
it was semantically difficult to use and i needed a lot more than just overriding registry(tag and org path) |
|
|
||
| func init() { | ||
| addonsEnableCmd.Flags().StringVar(&images, "images", "", "Images used by this addon. Separated by commas.") | ||
| addonsEnableCmd.Flags().StringVar(®istries, "registries", "", "Registries used by this addon. Separated by commas.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nirs @afbjorklund what do you think
Signed-off-by: Karan <[email protected]>
Signed-off-by: Karan <[email protected]>
77d38bc to
c834f61
Compare
|
rebased to master |
| if registries != "" { | ||
| viper.Set(config.AddonRegistries, registries) | ||
| } | ||
| if imageRepository != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am mor ein favor of making things more simple and not add one more flag, I rather if we can just have one flag for both, (we can add an alias for a flag to match helm)
what is helms exact name and documetnation
Summary
Adds
--image-repositoryflag tominikube addons enablefor Helm-based addons. Supports formatregistry/org[:tag]to override container images.Usage
Changes
Fixes #21257