-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat: Add securityContext. Fixes #96 #185
feat: Add securityContext. Fixes #96 #185
Conversation
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
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.
Looking good! Only thing that I'd like to change is to make the security context values optional and wrap the contexts with an if statement.
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
baa60b9
to
59e91a4
Compare
@seanson PTAL |
charts/argo-cd/templates/argocd-application-controller/deployment.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
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.
Tested locally, looking good. Thanks @davidkarlsen!
* make securityContext optional * add docs * bump chart version Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
@@ -25,6 +25,9 @@ spec: | |||
app.kubernetes.io/component: {{ .Values.redis.name }} | |||
spec: | |||
automountServiceAccountToken: false | |||
{{- if .Values.global.securityContext }} | |||
securityContext: {{- toYaml .Values.global.securityContext | nindent 8 }} | |||
{{- 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.
Hi all,
That PR didn't add a similar entry for templates/dex/deployment.yaml
with a global.securityContext
. Was that simply an omission that we should fix?
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 guess it was just forgotten - does it work with a similar security-context? I can add a PR.
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.
Yes, I think so. I've been using the same lines in dex/deployment.yaml
as what you had here for redis/deployment.yaml
and it's been working fine. Thanks for the PR.
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.
Signed-off-by: David J. M. Karlsen david@davidkarlsen.com
Adds security-context.
I wonder if it makes sense to reuse some other charts for dex / redis to avoid reinventing the wheel for these?
Checklist:
Chart.yaml
following Semantic Versioning.Changes are automatically published when merged to
master
. They are not published on branches.This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)