-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add secrets for pulling custom connector images #20272
Conversation
@@ -466,6 +466,8 @@ public interface Configs { | |||
*/ | |||
String getJobKubeMainContainerImagePullSecret(); | |||
|
|||
Optional<String> getCustomImagePullSecret(); |
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 think instead of adding a brand new config, it would make more sense for our OSS users if we instead updated the existing config to support a list.
For an OSS user who wants to use custom images, they're supposed to use the existing config, so having a second config will add confusion.
Basically, we just need to add support for defining multiple image pull secrets, so that Cloud can use two private image repositories instead of being limited to one.
Does that make sense?
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.
Makes sense, good call! I'll fix this.
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.
Thanks! Sorry to make you rework it all 😅
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.
Fixed - and retested on dev, this should work! (though some other bugs were detected, PR coming in...)
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.
Looks good @xiaohansong! I left a few nits about making variable names plural. I was wondering if we should change the environment variable from JOB_KUBE_MAIN_CONTAINER_IMAGE_PULL_SECRET
to JOB_KUBE_MAIN_CONTAINER_IMAGE_PULL_SECRETS
but it might not be worth it since it'd be a breaking change for OSS deployments that use it (and we'd need to update the environment variables in Cloud too, though I'm more concerned with breaking changes in OSS)
I think renaming our code variables and method to be plural while leaving the environment variable alone is a good middle ground, at least in my opinion.
What
Add secrets for pulling custom connector images
How
Adding an optional config field and pass it along to the pod creation process. Pod can accept them as a list and can pull images from both repositories if configured.