-
Notifications
You must be signed in to change notification settings - Fork 92
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
Make cleanup-leases security context configurable #587
base: main
Are you sure you want to change the base?
Make cleanup-leases security context configurable #587
Conversation
5ee062e
to
9c0675c
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.
need rebase
and need to add the possible or sample values for the security context in a comment in the values.yaml for users to know what they can use
c32ccd6
to
de1e772
Compare
@@ -25,6 +25,12 @@ spec: | |||
- -c | |||
- kubectl delete leases --all --ignore-not-found -n {{ .Release.Namespace }} | |||
restartPolicy: OnFailure | |||
{{- if .Values.leasescleanup.securityContext.enabled }} |
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.
you might dont need the enabled variable, just to check if the object exists should be fine
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 trying to conform to the style used elsewhere in the helm chart, I'm happy to change it however if consistency isn't as important
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.
ok, sgtm to keep
4125424
to
3269ce5
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.
LGTM overall, just few minor changes.
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 update the code to check Hector's comments
thank you!
The SecurityContext field for this job is currently static, however when deploying policy-controller into a namespace that uses Pod Security Admission controllers this job will not be able to run. Signed-off-by: Simon Witheridge <[email protected]>
3269ce5
to
8ea867a
Compare
@sirisaacnuketon Please, could you fix the conflicts? |
Description of the change
The SecurityContext field for this job is currently static, however when deploying policy-controller into a namespace that uses Pod Security Admission controllers this job will not be able to run.
This adds a new field to the values file that operates in a manner similar to the
securityContext
option for the webhook deployment.Existing or Associated Issue(s)
Additional Information
Checklist
Chart.yaml
according to semver. Where applicable, update and bump the versions in any associated umbrella chartvalues.yaml
and added to the README.md. The helm-docs utility can be used to generate the necessary content. Usehelm-docs --dry-run
to preview the content.ct lint
command.