-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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: enable securityContext
on spire server pod
#27363
Feat: enable securityContext
on spire server pod
#27363
Conversation
securitycontext
on spire server pod
securitycontext
on spire server podsecurityContext
on spire server pod
Commit 0e68c6f5661f8c87d50103b6c07b06734db11cfb does not contain "Signed-off-by". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
0e68c6f
to
959e531
Compare
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.
Thanks for the PR! The change itself looks good. I think the PR should be squashed into one commit though, please do that and re-request review when you have repushed branch, I will run CI then.
96a2d37
to
170b69d
Compare
Commit 5dee268eb610644b91f3640d5f9401146271e3f5 does not contain "Signed-off-by". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
5dee268
to
c627c7d
Compare
Thank you for your response, I have squashed my commits and pushed it. Just curious if we could also use squash and commit while merging this PR in case this is needed for the future too? |
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.
@ishuar we don't use squash and merge from GH because we usually want PRs to be structured logically. If the change is big, we want to have multiple commits, but your change simply seemed too fragmented. So we end up rebasing most PRs to have clean git history.
I can still see merge commit in your PR, can you get rid of it?
c627c7d
to
8fdc139
Compare
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.
Not sure why @cilium/sig-hubble was required for review, probably an older revision of the patch. Helm changes LGTM, pulling in @mhofstetter to take a look at the spire stuff.
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.
wrong button 😅
6a755e0
to
ffe8d28
Compare
283efef
to
8f31bfb
Compare
/test |
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.
I get an error running the SPIRE server locally on kind (see inline comment).
We might want to change the default?
8f31bfb
to
0c3cbf1
Compare
/test |
0c3cbf1
to
7bbc609
Compare
/test |
- Updated docs (used gnu-sed in macOS for helm-values.rst) permanent fix in cilium#27495 Signed-off-by: ishuar <ishansharma887@gmail.com>
7bbc609
to
74d48e7
Compare
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.
LGTM for me
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.
LGTM - thanks for the contribution.
/test |
Check "Conformance GatewayAPI" fails on Marking this PR as |
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXX
line if the commit addresses a particularGitHub issue.
Fixes: <commit-id>
tag, thenplease add the commit author[s] as reviewer[s] to this issue.