-
Notifications
You must be signed in to change notification settings - Fork 34
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 flagd sidecar resources attribute #514
Conversation
df1f228
to
e66aee1
Compare
Codecov Report
@@ Coverage Diff @@
## main #514 +/- ##
==========================================
+ Coverage 82.70% 82.78% +0.07%
==========================================
Files 22 22
Lines 1417 1423 +6
==========================================
+ Hits 1172 1178 +6
Misses 207 207
Partials 38 38
Flags with carried forward coverage won't be shown. Click here to find out more. |
Thank you again for your contribution. We already have the ability to configure flagd resource limits. But this is done through operator startup parameters 1 2. However, these configurations seems to be not well documented and difficult to manipulate the K8s descriptors. Hence, I see the proposed approach through this PR to superior. However, I think we can still have default resource requests/limits defined through current configurations and later override them through proposed change. @L3o-pold if you have time, you can write tests to cover this overriding behavior cc - @toddbaert & @thisthat appreciate input Footnotes |
@Kavindu-Dodan My current PR continues to use resources limits defined on the operator level but allow custom resources limits (override). |
Hey @L3o-pold, could you please update the FlagSourceConfiguration docs with an example? |
Yup, thank you for noticing this +1 for @beeme1mr's point on doc update |
e66aee1
to
6c670d1
Compare
9bd24c8
to
00486b0
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.
Approving
Let's wait for #513 and rebase before the merge
Signed-off-by: Leopold Jacquot <leopold.jacquot@infomaniak.com>
00486b0
to
d88e783
Compare
I agree with your suggestion @Kavindu-Dodan 👍 |
@thisthat thank you for noticing this. I think we can follow-up on this improvement as well as the test scenario I described later on I will go ahead with the merging this PR |
Created a follow-up for this #517 |
This PR
resources
attribute for FlagSourceConfiguration CRD to configure flagd sidecar resourcesRelated Issues
Fixes #511
Notes
Follow-up Tasks
How to test