-
Notifications
You must be signed in to change notification settings - Fork 17
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
support new registry option #45
Conversation
Let users specify the docker registry they wish to use when deploying addons. This facilitates offline deployments.
get-addon-templates
Outdated
|
||
content = content.replace("cdkbot", "{{ registry|default('cdkbot') }}") | ||
content = content.replace("k8s.gcr.io", "{{ registry|default('k8s.gcr.io') }}") | ||
content = content.replace("nvidia", "{{ registry|default('nvidia') }}") |
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.
The content-wide replacement of nvidia
is scary. Consider nvidia-device-plugin.yml - replacing nvidia
in here will butcher the template pretty bad.
Not that the existing code in here is a shining example of anything. The same concern applies to content-wide replacements of amd64
, etc. But the nvidia
case seems particularly bad - I don't think we can ship this.
Doing a replace on something like image: nvidia/
might be more acceptable, but still dirty. Probably more ideal would be to yaml.load
the data, walk through and find the image
fields, and only modify those.
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.
DOH! great catch @Cynerva. I put 'nvidia' in there as an afterthought (not testing on gpu-enabled hardware). i'll play around with the 'image: ' munging.
@Cynerva I pushed a version that does the replacement on Edit: i know it's inefficient to keep doing |
FYI, testing looks like this. Deploy cdk with k8s-master using a cdk-addons snap with this PR built-in, check the addon images:
Set the cdk-addons registry with
|
LGTM |
LGTM +1 |
We might want this pushed to 1.11 branch of cdk addons |
LGTM
Good catch, thanks. Merging this to master will only affect CI builds for 1.12 and onward. We probably want to cherry-pick this into |
Automatic merge from submit-queue (batch tested with PRs 66095, 66092). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. make the addons docker registry configurable **What this PR does / why we need it**: Allow users to configure the docker registry used when applying cdk-addons templates. **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes # **Special notes for your reviewer**: Depends on charmed-kubernetes/cdk-addons#45 **Release note**: ```release-note Expose docker registry config for addons used in Juju deployments ```
* support new registry option Let users specify the docker registry they wish to use when deploying addons. This facilitates offline deployments. * dont be so loose with content replacement * gah! forgot the content!
* support new registry option Let users specify the docker registry they wish to use when deploying addons. This facilitates offline deployments. * dont be so loose with content replacement * gah! forgot the content!
* support new registry option Let users specify the docker registry they wish to use when deploying addons. This facilitates offline deployments. * dont be so loose with content replacement * gah! forgot the content!
support new registry option (#45)
support new registry option (#45)
Let users specify the docker registry they wish to use when deploying
addons. This facilitates offline deployments.