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 support for security context #87

Merged
merged 2 commits into from
Mar 15, 2023
Merged

Conversation

mowies
Copy link
Contributor

@mowies mowies commented Mar 13, 2023

This PR

  • adds the (container) security context to the chart generation

@mowies mowies marked this pull request as ready for review March 13, 2023 12:42
@mowies
Copy link
Contributor Author

mowies commented Mar 13, 2023

@arttor this PR should be ready for review :)

@arttor
Copy link
Owner

arttor commented Mar 14, 2023

@mowies looks really nice and ready to merge. i am just thinking is there any case when chart maintainer doesn't want securityContext to be modified. In this case we should add flag and add securityContext to template only if it is enabled. what do you think?

@mowies
Copy link
Contributor Author

mowies commented Mar 14, 2023

I don't really see a use case for the CLI flag honestly.. I think if you have the security context set up, you will probably also want to show those values in the helm chart. Maybe we can leave that for now and see if users request it in the future?
Should be very easy to add in the future as well.

arttor
arttor previously approved these changes Mar 15, 2023
@arttor
Copy link
Owner

arttor commented Mar 15, 2023

@mowies can you please update your branch? i cannot merge pr due to conflicts

@mowies
Copy link
Contributor Author

mowies commented Mar 15, 2023

@arttor done

@arttor
Copy link
Owner

arttor commented Mar 15, 2023

@mowies still can't
image

@mowies
Copy link
Contributor Author

mowies commented Mar 15, 2023

ok this is very weird. for me, it shows everything green with no conflicts. i will do a full rebase...

Screenshot 2023-03-15 at 10 04 43

@arttor
Copy link
Owner

arttor commented Mar 15, 2023

strange, i still can't merge. can you try to rebase main?

@mowies
Copy link
Contributor Author

mowies commented Mar 15, 2023

ok, all done. seem that some examples were not fully re-generated with the last PR. I regenerated both the operator and the sample app again.

@arttor arttor merged commit 81ba5a2 into arttor:main Mar 15, 2023
@mowies mowies deleted the securitycontext branch March 15, 2023 09:49
@arttor
Copy link
Owner

arttor commented Mar 15, 2023

@mowies thank you! Your changes are available in v0.3.29 release

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