feat: add support for more deployment configuration #39
Conversation
matt-demers
requested review from
rchincha,
andaaron,
eusebiu-constantin-petu-dbk and
Andreea-Lupu
as code owners
Adds some standard deployment configuration values such as deployment annotations, priorityClassName, pod labels, dns configuration, and extraArgs for the container.
matt-demers
force-pushed
the
deployment-improvements
branch
from
17c3f23
to
fc94e6b
Compare
|
@matt-demers what was your use case motivating this PR? |
|
I upvoted because I like the ability to specify custom labels. It helps for example with log processing (enrichment of logs). While it is possible to do this while processing logs, standardizing labels for pods helps to keep it simple. |
There was a problem hiding this comment.
Overall I think it looks good and I would welcome the change.
I just added a nit because the default dnsPolicy might not be wanted if the default ever changes for Kubernetes. I still approve since at time of writing it non-breaking and I am not aware of any such change being planned in the future (and it would anyway be a breaking change for Kubernetes itself)
| dnsConfig: | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} | ||
| dnsPolicy: {{ .Values.dnsPolicy }} |
There was a problem hiding this comment.
I'm curious about the reasoning of not having dnsPolicy shielded with a with block like for the others. If in the future Kubernetes default ever changes it might be unexpected to have a default specified in the values of the helm chart?
There was a problem hiding this comment.
I was just following convention from other public helm charts that support this feature. For example the argocd helm chart has the dnsConfig in a with block but the dnsPolicy is not https://github.com/argoproj/argo-helm/blob/main/charts/argo-cd/templates/argocd-application-controller/deployment.yaml
We want to install the chart in our clusters, but we need the extra configuration options to fit within our cluster policies. We can work around it right now with some hacks by modifying the deployment after it is installed, but having the options in the chart itself would be much nicer. For instance, we want to use the chainguard provided image of zot, but that image does not have the cli args baked into the dockerfile, so we need to be able to set the args ourselves. |
|
@matt-demers Thanks for clarifying. DCO checks are failing in our CI. Missing sign-off(s): Add a git "sign-off" and also gpg sign your commit. |
| @@ -3,4 +3,4 @@ appVersion: v2.0.3 | |||
| description: A Helm chart for Kubernetes | |||
| name: zot | |||
| type: application | |||
| version: 0.1.53 | |||
| version: 0.1.54 | |||
There was a problem hiding this comment.
Please rebase and increment again. We just made a zot release and the new app version was included in .54
Adds some standard deployment configuration values such as deployment annotations, priorityClassName, and extraArgs for the container.
What type of PR is this?
feature
Which issue does this PR fix:
N/A
What does this PR do / Why do we need it:
Adding some common helm values for configuring the deployment that we need.
Testing done on this change:
Manually ran helm template on the chart to ensure that the values are being set properly
Will this break upgrades or downgrades?
No
Does this PR introduce any user-facing change?:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.