-
Notifications
You must be signed in to change notification settings - Fork 154
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
Add security context at pod and container level for Kanister operator #3075
base: master
Are you sure you want to change the base?
Conversation
6836d0d
to
5d30468
Compare
@julio-lopez please test it . |
@viveksinghggits could you please look into this PR sir ? |
Can you please add the test plan. We have a helm test written here Also, you don't have to say sir, just Vivek is fine. |
Usually when someone raises a PR it's their responsibility to add proper test plan and get help to test the changes. Like suggested above, let's add some tests and we can try to merge. |
yes sure. Thank you |
57f61ca
to
9d9ccd1
Compare
@viveksinghggits could you please take a look ? |
9d9ccd1
to
7ce7434
Compare
7ce7434
to
fe1a4b7
Compare
@viveksinghggits Thanks for your review. I think now it's look fine according to your recommendation. |
1ff8179
to
3aeb47d
Compare
{{- if .Values.controller.containerSecurityContext }} | ||
securityContext: | ||
{{ toYaml .Values.controller.containerSecurityContext | indent 10 }} | ||
{{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's create a function is _helpers.tpl
and use that at both the places for containers. We can do the same for deployment as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anishbista60 I see you have resolved this comment, but I don't see the changes for the deployment security context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@viveksinghggits I did it .
f06c4b4
to
d207e06
Compare
@viveksinghggits shall i go for removing that 2nd and 3 rd test cases ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anishbista60 so where we commit/push files to github, we should have a newline at the end of the file. It's general feedback for the files in this PR. Can you please make sure that they are committed properly so that they have the new line at the end.
d207e06
to
5f5302a
Compare
…erator Signed-off-by: Anish Bista <anishbista053@gmail.com>
Signed-off-by: Anish Bista <anishbista053@gmail.com>
Signed-off-by: Anish Bista <anishbista053@gmail.com>
5f5302a
to
3b4c154
Compare
now , i think looks good . Could you please reply on above in unresolved review ? |
Anyone here please take a look and try to get it merge. |
Change Overview
Expose securityContext on kanister-operator deployment to improve security
Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan
make helm-test
to verify that helm chart is working fine.