chore: allow specifying revision history limit for deployments #8094
Conversation
| @@ -1,3 +1,4 @@ | |||
| {{- $revisionHistoryLimit := or .Values.django.revisionHistoryLimit .Values.revisionHistoryLimit }} | |||
There was a problem hiding this comment.
Hi, so maybe I wasn't clear in my proposal about the simplification. I was thinking just to have one definition like .Values.revisionHistoryLimit and ommit under specific deployments in values. Like is in the e.g . https://github.com/bitnami/charts/blob/main/bitnami/apache/values.yaml .
There was a problem hiding this comment.
I think this is actually an even better solution than just exposing a single value. If you want to set a standard value for all deployments it works exactly as you would want, while also providing more flexibility to users that want to specify different limits for different parts of the overall deployments 🤔
There was a problem hiding this comment.
Not sure, helm as is already complicated, @alles-klar what is your opinion?
There was a problem hiding this comment.
It is more flexible but also more complex. If there is a use case for it, the flexible one is ok. Personally can't imagine a relevant use case for this. I mean it is just the revision history and not nothing imported to customize per deployment.
There was a problem hiding this comment.
I felt that this was a pretty flexible and un-opinionated method that I have used in other helm charts in the past. I figured it might be a good fit here. Since you all have strong opinions on that topic then I will defer. Please take another look.
There was a problem hiding this comment.
fwiw i left the conditional rendering (but not the conditional assignment) in so that users who did not specify this value in any other customizations. This will keep any templates from being unexpectedly modified by an opinionated statement of the default history limit by these templates and defer to Kubernetes's defaults. Hope that is okay.
| @@ -11,6 +11,9 @@ metadata: | |||
| helm.sh/chart: {{ include "defectdojo.chart" . }} | |||
| spec: | |||
| replicas: {{ .Values.celery.beat.replicas }} | |||
| {{- if .Values.revisionHistoryLimit }} | |||
There was a problem hiding this comment.
Just final polishing, so values by default parameters is disabled so then value is 10. What is the reason with this approach you are not using {{ .Values.revisionHistoryLimit | default 10 }} to save 6 LoC of flow control. It is not a big deal but code would be more readable
We are narrowing the scope of acceptable enhancements to DefectDojo in preparation for v3. Learn more here:
https://github.com/DefectDojo/django-DefectDojo/blob/master/readme-docs/CONTRIBUTING.md
Description
Allows setting the number of revisions of a replicaset maintained by a deployment in Kubernetes. Sets a default value of retained revisions for each deployment (worker, beat, and django).
Test results
n/a as this only touches the helm templates.
Documentation
The values file is updated with sane defaults.
Checklist
This checklist is for your information.
dev.dev.