fix(edge): Fixes enterprise helm chart to allow config of livenessProbe #671
Conversation
devanbenz
commented
Aug 28, 2024
devanbenz
commented
- Per our helm chart documentation consumers should be able to configure the livenessProbe
- This adds functionality to let users actually do that
| # path: "/ping" | ||
| # initialDelaySeconds: 30 | ||
| # timeoutSeconds: 5 | ||
| # scheme: HTTP |
There was a problem hiding this comment.
Since this scheme isn't consumed I think this might result in an YAML error for the templating engine through helm.
I've not tried this myself, but at a quick glance seeing that .Values.livenessProbe.scheme isn't used within the scheme field, it might be problematic if someone tries to use it from this example. I suspect the default can be HTTP in a similar manner to the /ping path
There was a problem hiding this comment.
Same with timeoutSeconds
There was a problem hiding this comment.
Does the same also need to be placed onto the meta nodes too?
Perhaps that wasn't the original ask, but a slight rejig to allowing something like:
# values.yaml
data:
livenessProbe:
...
meta:
livenessProbe:
...might be quite worthwhile? I'm not authoritative on this though of course 😄
For reference, as the livenessProbe field exists in both files:
rg "livenessProbe" templates -l
templates/meta-statefulset.yaml
templates/data-statefulset.yaml
There was a problem hiding this comment.
I'll go ahead and add to the meta nodes too
There was a problem hiding this comment.
@jdockerty should I be splitting up the livenessProbe or maybe just have a single value in the helm chart? If customers indicate they want it split we can do so but as of right now I think the customer is just wanting to adjust the data nodes.
There was a problem hiding this comment.
customer is just wanting to adjust the data nodes
In this case I think going to satisfy what a customer wants is fine, as it is a quick win, although it isn't my call too unfortunately. Might be worth checking with Geoffrey on that one 😄
| # path: "/ping" | ||
| # initialDelaySeconds: 30 | ||
| # timeoutSeconds: 5 | ||
| # scheme: HTTP |
There was a problem hiding this comment.
Same with timeoutSeconds
|
@jdockerty @jenniferplusplus I've modified this pr to only adjust per the requirement from our customer 🫡 |
