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
213 stars 12 forks source link

Allow Probes with value `null` #202

Closed matthias4217 closed 1 year ago

matthias4217 commented 1 year ago

I've got quite a specific use-case : I create Helm charts to be used by end-users. They have been designed so that they mostly have to edit values in helm.config.specific, and not directly Kubernetes objects. While this approach works mostly fine with Hull, there is a small issue about probes. My current way of handling them would be the following :

hull:
  config:
    specific:
      probes:
        liveness: ...
  objects:
    deployment:
      my-deployment:
        containers:
          my-container:
            livenessProbe: _HT*hull.config.specific.probes.liveness

However, if hull.config.specific.probes.liveness is {}, I get an error, as the probe is invalid according to Kubernetes. If it is null, I get another error, but from the helm linter, as null is not allowed in the JSONschema.

Of course, hull provides a workaround : let the user declare their probes directly in hull.objects. I could go with this solution, but I think it may not be a bad idea to authorize null values in Probes in the JSONschema.

gre9ory commented 1 year ago

Hi Matthias - wild guessing your name here ;)

Kubernetes API logic for this seems to enforce:

as a value for a probe key (livenessProbe, readinessProble, startupProbe) or it allows leaving out the xyzProbe key altogether.

K8S API itself does accept a xyzProbe: null in YAML (which I just verified) so then the schema should follow along the lines really. It might even be called a tiny problem with the automated Kubernetes API JSON schema generation which prevents something on the schema side which actually is allowed on the API side.

Anyhow, I am seeing absolutely no problem with the change and again thanks for the contribution! And all things considered I am always willing to sacrifice some schema strictness for the sake of better usability, I hope the schema validation is a good help for getting confiugration on the right track but it should not limit use cases such as yours really.

If you can wait a little, there is the 1.26 release of Kubernetes upcoming in two days and I will likely bring out a new batch of HULL releases later this week to match this. It will include this fix then.

gre9ory commented 1 year ago

And regarding your overall scenario it is how I think the concept is used to the fullest extent:

Add convenience abstractions such as a shared default probe configuration under hull.config.specific but still have direct access to each individual probe property so you can set a different probe for maybe only one component just in case you need that flexibility.

gre9ory commented 1 year ago

Change included in new batch of releases:

hull-1.26.0 hull-1.25.11 hull-1.24.12

For this new batch of releases initiated by Kubernetes 1.26 release I also added an automated build of a hull-demo chart for showcasing HULL features and bootstraping a HULL based Helm Chart, maybe you find it helpful, it is available in the Release section of this page.