dexidp
feat(charts/dex): Add extraObjects
Overview
This PR add the ability to include extraObjects. Array of extra K8s manifests to deploy
What this PR does / why we need it
Closes #3227
Special notes for your reviewer
Checklist
- [x] Change log updated in
Chart.yaml(see the contributing guide for details) - [x] Chart version bumped in
Chart.yaml(see the contributing guide for details) - [x] Documentation regenerated by running
make docs
I opened another PR https://github.com/dexidp/helm-charts/pull/132 on this a long time ago as well. I know I'm biased, but I like my solution more as it allows extraObjects to be a dictionary or array. Useful for defining multiple extraObjects from different values files, such as one encrypted and another not.
@TheRealNoob do you have any references from the community how other charts solve this? I'd be inclined to follow the footsteps of others.
I've not run across any other charts currently allowing extraObjects to be a dictionary, only a list; let alone either. With that in mind I understand your point, but I see this as a feature add with zero risk.
I understand the stance you're making, and I've made it myself many a time. In this instance I'm stating that I believe my change to have no negative drawbacks for anybody using their values file in the way your PR works, while also adding a feature that I believe others (and myself) would find useful. If you believe anything in that statement to be incorrect please let me know. If that isn't the issue, is there some other reason why my PR is undesired (other than it being different)?
I'm in favour of PR #132 over this. I prefer being able to define extraObjects as a dict as it allows for yaml merging across multiple values files.
Well, I have several examples for this:
Notifik (as a list) App template (as a map) Varnish (as a list)
Personally, i agree with @skhtor. Maps allow you to merge between files and it's easy to implement too, so they are more useful
So ... any news on this ? I close this one and would be great to make something for #132, we need this ! @TheRealNoob
