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

[Application]: add command to container and sidecars and fix security context (again) #164

Merged
merged 13 commits into from
Dec 3, 2024

Conversation

mhaxp
Copy link
Contributor

@mhaxp mhaxp commented Nov 28, 2024

We trapped into helm/helm#5238 with the merge of default security context with specific ones in sidecar or initcontainer.

objects seem to be merged by using the or operation - effectively making true always the result for bool properties.

with this change, behavior changes, but as it was already wrong in the past it just can get better.

@mhaxp mhaxp requested a review from a team as a code owner November 28, 2024 14:45
@technolinator-sbom-as-a-service
Copy link

technolinator-sbom-as-a-service bot commented Nov 28, 2024

🏆 No vulnerabilities found

@stuebingerb
Copy link
Contributor

Maybe it makes sense to add a test as well?

@mhaxp
Copy link
Contributor Author

mhaxp commented Nov 29, 2024

Question: does this overrides the container defaults?

args: {{ if not .Values.container.args }}[]{{ end }}

@beiertu-mms
Copy link
Contributor

please add a reference to the chart in the PR title, similar to

Or use conventional commits, for example feat(application): add command to container and sidecars

@mhaxp mhaxp changed the title add commnd to container and sidecars feat(application): add commnd to container and sidecars Nov 29, 2024
@stuebingerb
Copy link
Contributor

please add a reference to the chart in the PR title, similar to

Or use conventional commits, for example feat(application): add command to container and sidecars

+1
Would be good to have this documented somewhere, though 😉

@mhaxp mhaxp requested a review from heubeck November 29, 2024 14:20
@mhaxp mhaxp enabled auto-merge (squash) December 2, 2024 09:52
@mhaxp mhaxp disabled auto-merge December 2, 2024 10:42
@mhaxp mhaxp changed the title feat(application): add commnd to container and sidecars feat(application): add command to container and sidecars Dec 2, 2024
@heubeck
Copy link
Member

heubeck commented Dec 3, 2024

Question: does this overrides the container defaults?

args: {{ if not .Values.container.args }}[]{{ end }}

only if given an non-empty list, I would assume.

heubeck
heubeck previously approved these changes Dec 3, 2024
@heubeck heubeck enabled auto-merge December 3, 2024 07:18
@heubeck heubeck disabled auto-merge December 3, 2024 07:19
@heubeck heubeck enabled auto-merge (squash) December 3, 2024 07:23
@heubeck heubeck disabled auto-merge December 3, 2024 07:24
heubeck
heubeck previously approved these changes Dec 3, 2024
Copy link
Contributor

@beiertu-mms beiertu-mms left a comment

Choose a reason for hiding this comment

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

sorry, I have a few more nitpicks

@heubeck heubeck changed the title feat(application): add command to container and sidecars [Application]: add command to container and sidecars and fix security context (again) Dec 3, 2024
@heubeck heubeck force-pushed the mha_command branch 2 times, most recently from 4fa0719 to 5aacf7b Compare December 3, 2024 10:09
@heubeck heubeck disabled auto-merge December 3, 2024 10:19
@heubeck heubeck enabled auto-merge (squash) December 3, 2024 10:19
@heubeck heubeck merged commit e520c3a into main Dec 3, 2024
9 checks passed
@heubeck heubeck deleted the mha_command branch December 3, 2024 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants