vidispine / hull

The incredible HULL - Helm Uniform Layer Library - is a Helm library chart to improve Helm chart based workflows
https://github.com/vidispine/hull
Apache License 2.0
231 stars 13 forks source link

Empty fields in rendered template #282

Closed alexrimlin closed 9 months ago

alexrimlin commented 9 months ago

Hi. First of all, thank you for this wonderful project. Hull generates empty values for fields that I don't provide, which is very undesired in the context of ArgoCD. With autosync set the application doesn't ever get synced entirely, given that it removes unused fields and I have no way of disabling these fields in hull's config. Is it possible to not render empty fields?

image
gre9ory commented 9 months ago

Hi @alexrimlin,

Hi. First of all, thank you for this wonderful project.

Thank you, glad you like it!

Hull generates empty values for fields that I don't provide, which is very undesired in the context of ArgoCD. With autosync set the application doesn't ever get synced entirely, given that it removes unused fields and I have no way of disabling these fields in hull's config. Is it possible to not render empty fields?

Special thanks for the image, very helpful. Are those all the fields you want to be removed or do you know of others which are bothering you?

Generally I think that should be no problem to remove these empty fields and may also be more appropriate default behavior to not render them, Looks like most of them come to be via one central function here which should be changeable to not render out empty arrays or dicts. For some downwards compatibility I would still like to allow to opt-in to rendering empty fields as in the current behavior?

I am gonna check on that and get back to you,

alexrimlin commented 9 months ago

Thank you for responding so promptly. A field such as .hull.config.general.omitEmptyCollections that is false by default but can be set to true would be perfect.

gre9ory commented 9 months ago

Sorry, whole family was ill all week so I did not have much time to dedicate to this. But I think this will be releasable start of next week.

alexrimlin commented 9 months ago

No need to be sorry. I highly appreciate that you support an open source project that has made helm super fun for me to use!

gre9ory commented 9 months ago

Hi @alexrimlin, first thank you for the nice feedback!

I chose to make leaving out the empty collection fields the new default behavior. Less clutter in my opinion and down the line it should not make a difference when Kubernetes handles the YAML. Nevertheless you can still opt in to the "old" style with rendered empty collections with these new options:

hull:
  config:
    general:  
      render:
        emptyLabels: false
        emptyAnnotations: false
        emptyTemplateLabels: false
        emptyTemplateAnnotations: false
        emptyHullObjects: false

You can define rendering styles for object metadata labels and annotations, pod template labels and annotations. Since pod template metadata changes are relevant for pod restarts I thought it may be better to differentiate this. Also note that technically the labels will probably never be empty due to the prepopulation of them and their overall technical importance but for consistency's sake I added options for them too.

All other collections that previously were always rendered empty fall under emptyHullObjects so far. These are:

If it were required for some reason to have more fine grained options, further settings could be amended for choosing the exact fields behavior at a later stage - but I guess this should be granular enough. An interesting aspect is that for ingress -> rules -> http.paths, the paths field is required in the JSON schema. So there would be a difference when you validate a rendered ingress YAML with empty path fields. Prior to the change this would pass validation but now with the pruned paths field it wouldn't. However I tested this and essentially Kubernetes does not care whether you have empty or missing paths, in both cases it fails and enforces at least one element is present inpaths. Similarily the containers in a pod must be present of course and at least one element must exist in it, otherwise it is not passing as a Kubernetes pod by definition.

In summary I did not have the feeling that changing the default is a breaking change, I hope that is agreeable and I did not miss any collection field, if so let me know please.

Thanks for the good idea, if you have more of those I am happy to hear about them. Maybe I'll start adding issues for feature ideas not yet implemented so there is some possibility for discussions before implementation starts.

Releases: 1.27.11 1.28.9 1.29.3