Skip to content
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

Merged
merged 7 commits into from
Dec 16, 2019

Conversation

davidkarlsen
Copy link
Contributor

@davidkarlsen davidkarlsen commented Dec 10, 2019

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:

  • I have update the chart version in Chart.yaml following Semantic Versioning.
  • Any new values are backwards compatible and/or have sensible default.
  • I have followed the testing instructions in the contributing guide.
  • I have signed the CLA and the build is green.
  • I will test my changes again once merged to master and published.

Changes are automatically published when merged to master. They are not published on branches.


This change is Reviewable

Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
@claassistantio
Copy link

claassistantio commented Dec 10, 2019

CLA assistant check
All committers have signed the CLA.

@davidkarlsen davidkarlsen changed the title Add securityContext. Fixes #96 feat: Add securityContext. Fixes #96 Dec 10, 2019
@seanson seanson added argo-cd enhancement New feature or request labels Dec 10, 2019
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
Copy link
Contributor

@seanson seanson left a 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>
@davidkarlsen davidkarlsen force-pushed the feature/securitycontext branch from baa60b9 to 59e91a4 Compare December 12, 2019 08:43
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
@davidkarlsen
Copy link
Contributor Author

@seanson PTAL

charts/argo-cd/values.yaml Outdated Show resolved Hide resolved
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
@davidkarlsen davidkarlsen requested a review from seanson December 15, 2019 23:14
Copy link
Contributor

@seanson seanson left a 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!

@seanson seanson merged commit c1f6ed4 into argoproj:master Dec 16, 2019
@davidkarlsen davidkarlsen deleted the feature/securitycontext branch December 16, 2019 05:59
nouseforaname pushed a commit to nouseforaname/argo-helm that referenced this pull request Jan 29, 2020
* 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 }}

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?

Copy link
Contributor Author

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.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
argo-cd enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants