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.

Sign up for GitHub

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

Jump to bottom

feat: adds securityContexts (pod & container) #20

Merged
merged 1 commit into from
Dec 14, 2022
Merged

feat: adds securityContexts (pod & container) #20

merged 1 commit into from
Dec 14, 2022

Conversation

ChrisJBurns
Copy link
Contributor

@ChrisJBurns ChrisJBurns commented Dec 8, 2022 •
edited
Loading

Description of the change

Checklist

  • Chart version bumped in Chart.yaml according to semver.
  • Variables are documented in the values.yaml and added to the README.md. The helm-docs utility can be used to generate the necessary content. Use helm-docs --dry-run to preview the content.
  • [N/A] JSON Schema generated.
  • List tests pass for Chart using the Chart Testing tool and the ct lint command.

Signed-off-by: ChrisJBurns 29541485+ChrisJBurns@users.noreply.github.com

@ChrisJBurns ChrisJBurns requested a review from a team as a code owner December 8, 2022 19:03
@ChrisJBurns ChrisJBurns force-pushed the adds-security-context branch 3 times, most recently from 1f99e75 to aea8d1a Compare December 8, 2022 20:13
sabre1041
sabre1041 requested changes Dec 12, 2022
View reviewed changes
Copy link
Contributor

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

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

Recommend using the common.tplvalues.render function instead of calling toYaml directly

@tumido tumido mentioned this pull request Dec 13, 2022
Merged
4 tasks
@tumido
Copy link
Member

tumido commented Dec 13, 2022

@sabre1041 can you please elaborate on that? I do not understand the benefits of common.tplvalues.render here. Should we use it instead of toYaml in all places? What's the benefit? Should we always expect a templated value? I'm quite uneducated on this topic so if you have the experience that this would be a helpful practice, we should adopt it.

@tumido
Copy link
Member

tumido commented Dec 13, 2022

@ChrisJBurns can you please rebase and run pre-commit run -a?

@ChrisJBurns ChrisJBurns force-pushed the adds-security-context branch from 86bfdee to 2c5ccba Compare December 13, 2022 14:22
@ChrisJBurns
feat: adds securityContext for pods and containers
fee32ef 
Signed-off-by: ChrisJBurns <29541485+ChrisJBurns@users.noreply.github.com>
@ChrisJBurns ChrisJBurns force-pushed the adds-security-context branch from 953622b to fee32ef Compare December 13, 2022 14:24
@ChrisJBurns
Copy link
Contributor Author

@tumido @sabre1041 @vinzscam Rebased, addressed toYaml comment, bumped Chart and pushed. 👍

@sabre1041
Copy link
Contributor

@tumido the partial basically wraps the functionality of toYaml and tpl into a convenient reusable function. By adding in tpl, there are enhanced capabilities that can be realized

Source: https://github.com/bitnami/charts/blob/main/bitnami/common/templates/_tplvalues.tpl#L11

sabre1041
sabre1041 approved these changes Dec 13, 2022
View reviewed changes
Copy link
Contributor

@sabre1041 sabre1041 left a comment

Choose a reason for hiding this comment

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

LGTM

@tumido tumido merged commit f89147a into backstage:main Dec 14, 2022
@pfuhrmann pfuhrmann mentioned this pull request Feb 6, 2023
Merged
1 task
vinzscam pushed a commit that referenced this pull request Feb 6, 2023
@pfuhrmann
Fix containerSecurityContext (#57)
8f188f5 
* Fix containerSecurityContext

Apply actually `containerSecurityContext` instead of `podSecurityContext` (I assume this a bug introduced in #20).

Signed-off-by: Patrik Fuhrmann <github@pinglf.com>

* Bump the chart version

Signed-off-by: Patrik Fuhrmann <github@pinglf.com>

* Fix README

Signed-off-by: Patrik Fuhrmann <github@pinglf.com>

* Bump only patch version instead of minor

Signed-off-by: Patrik Fuhrmann <github@pinglf.com>

---------

Signed-off-by: Patrik Fuhrmann <github@pinglf.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vinzscam vinzscam vinzscam approved these changes

@tumido tumido tumido approved these changes

@sabre1041 sabre1041 sabre1041 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🚀 Feature: Make it possible to set securityContext
4 participants
@ChrisJBurns @tumido @sabre1041 @vinzscam