-
Notifications
You must be signed in to change notification settings - Fork 44
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
Remove life cycle hook from Daemonset #110
Remove life cycle hook from Daemonset #110
Conversation
Signed-off-by: joshvanl <me@joshvanl.dev>
Signed-off-by: joshvanl <me@joshvanl.dev>
@@ -19,10 +19,6 @@ spec: | |||
- name: node-driver-registrar | |||
image: "{{ .Values.nodeDriverRegistrarImage.repository }}:{{ .Values.nodeDriverRegistrarImage.tag }}" | |||
imagePullPolicy: {{ .Values.image.pullPolicy }} | |||
lifecycle: |
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.
Just curious, why was this needed before? and why it's no longer 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.
Please follow the issue here
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 🤦♂️ :)
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 Josh, this makes sense
I've added a question, apart from that happy for this to be merged.
/lgtm
/hold
lifecycle: | ||
preStop: | ||
exec: | ||
command: ["/bin/sh", "-c", "rm -rf /registration/cert-manager-csi-driver /registration/cert-manager-csi-driver-reg.sock"] |
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.
This command removes both plugin (?) and socket whereas the fix in kubernetes-csi/node-driver-registrar#61 seems to only remove the socket- do we think that might be an issue?
I do see that the kubernetes-csi/node-driver-registrar#61 removes this command from the example deployment, so assume that this is considered no longer 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.
That it my understanding as well.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: irbekrm, JoshVanL The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/hold cancel |
Add go module and replace oci-image with oci-build and oci-publish
Fixes #109
See kubernetes-csi/node-driver-registrar#21 and kubernetes-csi/node-driver-registrar#61
/cc @munnerz
Also updates the helm chart version ready for published release.