-
Notifications
You must be signed in to change notification settings - Fork 875
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow additional annotations for standby and active services via config #896
Allow additional annotations for standby and active services via config #896
Conversation
This is a must have feature for Vault HELM chart to set difference annotations for individual services, how long we can see this appoved? |
e6527d0
to
663a24e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tekicat -
Thanks for submitting this, I just got around to reviewing. I think the approach looks good, and is a nice+clean iteration of the original PR.
I left a few suggestions about missing tests, and some general cleanup, and after these get addressed I think we're good to go!
Let me know if you have any questions about any the feedback.
~Kyle
{{/* | ||
Sets extra vault server Service (active) annotations | ||
*/}} | ||
{{- define "vault.service.active.annotations" -}} | ||
{{- if .Values.server.service.active.annotations }} | ||
{{- $tp := typeOf .Values.server.service.active.annotations }} | ||
{{- if eq $tp "string" }} | ||
{{- tpl .Values.server.service.active.annotations . | nindent 4 }} | ||
{{- else }} | ||
{{- toYaml .Values.server.service.active.annotations | nindent 4 }} | ||
{{- end }} | ||
{{- end }} | ||
{{- end -}} | ||
{{/* | ||
Sets extra vault server Service annotations | ||
*/}} | ||
{{- define "vault.service.standby.annotations" -}} | ||
{{- if .Values.server.service.standby.annotations }} | ||
{{- $tp := typeOf .Values.server.service.standby.annotations }} | ||
{{- if eq $tp "string" }} | ||
{{- tpl .Values.server.service.standby.annotations . | nindent 4 }} | ||
{{- else }} | ||
{{- toYaml .Values.server.service.standby.annotations | nindent 4 }} | ||
{{- end }} | ||
{{- end }} | ||
{{- end -}} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was going to suggest that we dry this up a bit and make it a single function where we just pass in .Values.server.service.active/standby
as the root, but I ran into helm/helm#5979 trying to implement it myself, so I think it's okay as is.
Co-authored-by: Kyle Schochenmaier <kyle.schochenmaier@hashicorp.com>
Thanks for the suggestions @kschoche , they looked good and I have applied them. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much, this looks great!
Revival of #546
Currently there is no ability to annotate services for active and standby services, as they inherit their settings from the main server service configuration. This is a problem, when you need to attach different annotations to the different services. Instead of just allowing for setting of annotations, I opted to allow configuring the services for more flexibility.
Helps to close #674
Partially resolves #588
Thanks @jamesgoodhouse