-
Notifications
You must be signed in to change notification settings - Fork 248
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
best-practices+karpenter+other: Enhance policies by iterating over all container types #1054
base: main
Are you sure you want to change the base?
best-practices+karpenter+other: Enhance policies by iterating over all container types #1054
Conversation
cebc629
to
bba39a3
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.
Hi and thanks for the contribution. You've got two test failures here reported in CI. See if you can fix those areas of the two affected policies.
other/memory-requests-equal-limits/memory-requests-equal-limits.yaml
Outdated
Show resolved
Hide resolved
e5e9bd5
to
472666c
Compare
In this commit, we ensure policies apply to `ephemeralContainers`, `initContainers`, and `containers`. Signed-off-by: Mohamed Awnallah <[email protected]>
In this commit, we compute sha256 of the changed policies using `sha256sum` utility for integrity. Signed-off-by: Mohamed Awnallah <[email protected]>
472666c
to
428be01
Compare
Thanks a lot @chipzoller for the feedback! I've addressed all of it. The test cases all passed as you seen in my fork repo: |
FYI, after addressing comments, please resolve conversations so maintainers know what to expect on subsequent reviews. |
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.
This policy is not correctly updated to account for the addition of initContainers and ephemeralContainers. The context variable is still operating on just containers which is causing CI tests to fail.
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.
@mohamedawnallah - can you please resolve this, so we can merge?
Bump |
Related Issue(s)
Closes #1012
Description
Enhance policies by iterating over all container types i.e
ephemeralContainers
,initContainers
, andcontainers
.Context
This is my first PR to Kyverno Org! I would love to receive any feedback, thanks! 🙏
Checklist