-
Notifications
You must be signed in to change notification settings - Fork 31
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
Add KUBEVIP_NAMESPACE, KUBEVIP_CONFIG_MAP environment variables and c… #48
Add KUBEVIP_NAMESPACE, KUBEVIP_CONFIG_MAP environment variables and c… #48
Conversation
…onfigmap-checksum to redeploy workload Signed-off-by: Ibrahima COULIBALY <[email protected]>
…onfigmap-checksum to redeploy workload Signed-off-by: Ibrahima COULIBALY <[email protected]>
…onfigmap-checksum to redeploy workload Signed-off-by: Ibrahima COULIBALY <[email protected]>
Very nice, I was just about to look into this. Thanks a lot @MITCOULIB ! I hope it can be merged soon. |
spec: | ||
containers: | ||
- command: | ||
- /kube-vip-cloud-provider | ||
- --leader-elect-resource-name=kube-vip-cloud-controller | ||
image: {{ .Values.image.repository }}:{{ .Values.image.tag | default .Chart.AppVersion }} | ||
name: {{ include "kube-vip-cloud-provider.name" . }} | ||
env: | ||
- name: KUBEVIP_NAMESPACE | ||
value: {{ .Release.Namespace | default "kube-system" }} |
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 believe releases always have a namespace, so the default "kube-system" can never be applied, so it could be removed (dead code).
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.
@rptaylor yes absolutely
@@ -12,13 +12,20 @@ spec: | |||
metadata: | |||
labels: | |||
{{- include "kube-vip-cloud-provider.selectorLabels" . | nindent 8 }} | |||
annotations: | |||
configmap-checksum: {{ include (print $.Template.BasePath "/configmap.yaml") . | sha256sum }} |
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.
Is this related to https://helm.sh/docs/howto/charts_tips_and_tricks/#automatically-roll-deployments ?
I did a test and confirmed that kube-vip-cloud-provider will read updates to the configmap if it changes, so it doesn't need to be restarted if the configmap changes. This is consistent with the log messages indicating it uses watch
to detect configmap changes, so I don't think an annotation is needed.
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 for the confirmation. I removed the configmap-checksum annotations
Signed-off-by: Ibrahima COULIBALY <[email protected]>
Signed-off-by: Ibrahima COULIBALY <[email protected]>
Signed-off-by: Ibrahima COULIBALY <[email protected]>
Add KUBEVIP_NAMESPACE, KUBEVIP_CONFIG_MAP environment variables and configmap-checksum to redeploy workload