-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: Add image pull secrets #655
feat: Add image pull secrets #655
Conversation
I think I see the problem, I'll give a fix and shot and push up my change. Thanks for this! |
de4b05c
to
7b83fda
Compare
Signed-off-by: Todd Baert <[email protected]>
@cpitstick-latai I pushed some updates to your branch, I hope you don't mind (I had to sign off your commit). In addition, I added a single pullSecret, used by both the templates at the runtime components (we have a few runtime components that pull images). As we discussed, we can't in any clean way support a normal array of I tested this locally (setting and not setting it) and it works as expected (conveniently, an empty |
This is great. Feel free to take this over and submit it! |
One request I'll have is to add supporting the array of imagePullSecrets to the backlog for OpenFeature. Inevitably the build process will change and evolve, and when it does, this use-case will just become part of it. |
@toddbaert should we work on this now 🤔 Given the limitation on helm operators in kuztomize layer, a quick solution would be to rename Otherwise latest change looks good |
I wouldn't be opposed to that, but I think long term, a solution to the kustomize+helm mess is fundamentally in order. If you want to push the comma-separated solution onto this that's fine as well, it wouldn't take too much, but in a sense it's still a bit hacky since it's not really how to appropriately represent arrays in yaml. Not sure what @cpitstick-latai thinks. |
Signed-off-by: Todd Baert <[email protected]>
Signed-off-by: Todd Baert <[email protected]>
@toddbaert As to the idea for an imagePullSecrets comma-separated array, I would rather have the enhanced functionality than more limited, even if hacky. We're already arguing over two separate hacks due to the way this build system is broken. So, as long as it's well documented and works properly with 0-to-n secrets, I'm OK with that solution. But you should additionally create a backlog Epic to revisit the kustomize-helm build system, as I doubt this is the last time that will become a pain point! |
@Kavindu-Dodan and I discussed it a bit further and we don't think this is possible with the current system after all, since we could not handle the secrets used only in templates/manifests; we can only handle the runtime ones with this solution. I will likely merge this as is and release, but in addition I've opened: #665 |
DISCLAIMER: THIS DOES NOT BUILD AND I DO NOT KNOW WHY.
Tested with
make helm-package
and get this error: