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

Helm chart: SecurityContext configurable #1519

Closed
wants to merge 2 commits into from

Conversation

Gianluca755
Copy link
Contributor

  • Tests written and linted ℹ︎
  • Documentation written ℹ︎
  • Commit history is tidy ℹ︎

The above links are broken!

What this does

Resolve #1518.

@Gianluca755 Gianluca755 requested a review from a team as a code owner August 23, 2024 05:47
Copy link
Contributor

@jonnyowenpowell jonnyowenpowell left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

We would like to retain compatibility with the current securityContext.fsGroup value. As such, could I ask that the PR be updated to use and give precedence to that value, when it is set, as follows:

{{- with .Values.podSecurityContext }}
securityContext:
{{- $fsGroupOverride := dict }}
{{- if hasKey $.Values.securityContext "fsGroup" }}
{{- $fsGroupOverride = dict "fsGroup" (int $.Values.securityContext.fsGroup) }}
{{- end }}
{{- merge $fsGroupOverride . | toYaml | nindent 8 }}
{{- else }}
{{- if .Values.securityContext.fsGroup }}
securityContext:
  fsGroup: {{ int .Values.securityContext.fsGroup }}
{{- end }}
{{- end }}

@Gianluca755
Copy link
Contributor Author

@jonnyowenpowell like this?

Copy link
Contributor

@jonnyowenpowell jonnyowenpowell left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jonnyowenpowell jonnyowenpowell changed the base branch from master to staging October 1, 2024 09:42
@jonnyowenpowell jonnyowenpowell changed the base branch from staging to master October 1, 2024 09:43
@jonnyowenpowell
Copy link
Contributor

I've cherry-picked and raised this separately in the PR linked above to avoid back-and-forth on non-functional issues like targeting the staging branch and having commit messages pass linting.

Thanks very much for the contribution 🙏

I'll update here when it's deployed.

@jonnyowenpowell
Copy link
Contributor

This has been deployed with this release: #1525.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants