Skip to content
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

Merged
merged 7 commits into from
Jun 4, 2024

Conversation

cpitstick-latai
Copy link
Contributor

@cpitstick-latai cpitstick-latai commented May 31, 2024

DISCLAIMER: THIS DOES NOT BUILD AND I DO NOT KNOW WHY.

Tested with make helm-package and get this error:

Error: trouble configuring builtin PatchStrategicMergeTransformer with config: `
paths:
- exclude-ns.yaml
- manager.yaml
- exclude-validatingwebhook.yaml
`: MalformedYAMLError: yaml: line 9: did not find expected node content in File: manager.yaml
make: *** [Makefile:157: release-manifests] Error 1

@cpitstick-latai cpitstick-latai requested a review from a team as a code owner May 31, 2024 16:37
@toddbaert toddbaert self-requested a review June 3, 2024 12:51
@toddbaert
Copy link
Member

I think I see the problem, I'll give a fix and shot and push up my change. Thanks for this!

@cpitstick-latai cpitstick-latai changed the title Adding image pull secrets fix: Adding image pull secrets Jun 3, 2024
@toddbaert toddbaert changed the title fix: Adding image pull secrets feat: add image pull secrets Jun 3, 2024
toddbaert and others added 2 commits June 3, 2024 17:03
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@toddbaert
Copy link
Member

toddbaert commented Jun 3, 2024

@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 imagePullSecrets but I think this should cover most use-cases.

I tested this locally (setting and not setting it) and it works as expected (conveniently, an empty imagePullSecret is completely ignored.

@cpitstick-latai
Copy link
Contributor Author

This is great. Feel free to take this over and submit it!

@cpitstick-latai
Copy link
Contributor Author

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.

@Kavindu-Dodan
Copy link
Contributor

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 imagePullSecret to imagePullSecretNames where imagePullSecretNames can caontain a comma separated names to be input in the secret array.

Otherwise latest change looks good

@toddbaert
Copy link
Member

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 imagePullSecret to imagePullSecretNames where imagePullSecretNames can caontain a comma separated names to be input in the secret array.

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.

@toddbaert toddbaert requested a review from odubajDT June 4, 2024 16:19
toddbaert and others added 3 commits June 4, 2024 12:25
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
@cpitstick-latai
Copy link
Contributor Author

@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!

@toddbaert
Copy link
Member

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 imagePullSecret to imagePullSecretNames where imagePullSecretNames can caontain a comma separated names to be input in the secret array.

Otherwise latest change looks good

@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

@toddbaert toddbaert merged commit 2d7b30c into open-feature:main Jun 4, 2024
14 checks passed
@github-actions github-actions bot mentioned this pull request Jun 4, 2024
@cpitstick-latai cpitstick-latai deleted the add-image-pull-secrets branch June 5, 2024 18:47
@cpitstick-latai cpitstick-latai changed the title feat: add image pull secrets feat: Add image pull secrets Jun 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants