-
-
Notifications
You must be signed in to change notification settings - Fork 371
Add Resize subresource for Pod
#1851
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
Conversation
Signed-off-by: hugoPonthieu <[email protected]>
Signed-off-by: hugoPonthieu <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1851 +/- ##
=======================================
- Coverage 74.7% 74.6% -0.0%
=======================================
Files 84 84
Lines 7910 7912 +2
=======================================
Hits 5902 5902
- Misses 2008 2010 +2
🚀 New features to boost your workflow:
|
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 a lot for this. i think this is almost perfect, minor suggestion and question about a potential addition, but happy to merge at green (EDIT: oh, it passed fmt).
| if let Some(spec) = &mut current_pod.spec { | ||
| if let Some(container) = spec.containers.get_mut(0) { | ||
| if let Some(resources) = &mut container.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.
you can avoid some rightward drift here by using a let-chain
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.
Ty for this langage tips ! I did not know about it.
Done.
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 looks great! you might have to backout of it atm to merge the pr (see my normal comment below), but i can use your commit in a follow-up afterwards where i'm fixing rustfmt / build issues.
| info!("Example 3: Using replace_resize method"); | ||
| let mut current_pod = pods.get_resize("resize-demo").await?; |
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.
do you think it's possible and/or worth extending the conditions module to be able to wait for the PodResizePending or / PodResizeInProgress condition to complete?
i feel like if you are patching things like this you might want to "wait for it to complete", and if an await_condition hasn't succeeded in like 10s then maybe people would look into whether the request was "Infeasible" / "Deferred" themselves.
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 would like to implement this. Is it possible to create an issue from this comment ?
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.
sure thing, feel free to copy paste it before or after merge (or i can write one after merge). just leave this comment unresolved until we write one. 😄
|
ah ugh. i forgot we never finalised the rust 2024 stuff (which let-chains depend on) from rustdoc pov... sorry. feel free to backout your last change and i can fix this later / cherry pick your last change as part of the rustfmt upgrade (it has some build issues to try to do directly) |
dc30de0 to
de3b4f7
Compare
Motivation
Since 1.33 the resize of pods resources is available. The goal of this PRs is to provide a convenient way to do it.
This is linked to #1793
Solution
I implemented:
I followed was has been done in before in the subresources.