-
Notifications
You must be signed in to change notification settings - Fork 33
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 limits and requests to PolicyServer #708
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #708 +/- ##
==========================================
+ Coverage 44.97% 51.48% +6.51%
==========================================
Files 22 27 +5
Lines 1543 2049 +506
==========================================
+ Hits 694 1055 +361
- Misses 788 886 +98
- Partials 61 108 +47
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
f937ba3
to
61ae5b8
Compare
61ae5b8
to
bb2de58
Compare
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
bb2de58
to
9883709
Compare
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
9883709
to
8d58fcd
Compare
!reflect.DeepEqual(originalDeployment.Spec.Template.Spec.Containers[0].Env, newDeployment.Spec.Template.Spec.Containers[0].Env) || | ||
!reflect.DeepEqual(originalDeployment.Spec.Template.Spec.Containers[0].Resources, newDeployment.Spec.Template.Spec.Containers[0].Resources) || |
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 made me think... can we have 100% sure that the first container will be the policy server one? Cannot be the OPTEL collector sometimes?
If we are not sure about that, I think we should open an issue to investigate or maybe mitigate this.
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.
Good call, I would defer the changes here as we are probably going to refactor this by using controller-runtime CreateOrUpdate utilities in 1.13
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
Signed-off-by: Fabrizio Sestito <fabrizio.sestito@suse.com>
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
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.
As we discuss during the daily... LGTM
Description
#710
Adds limits and requests field to the
PolicyServer
resource.The fields are propagated to the PolicyServer deployment down to the running PolicyServer pods.
The PolicyServer webhook validates the fields and implements the default logic (if limits are specified and requests are left empty, the requests default to limits).
Testing
Integration tests and unit tests added