-
-
Notifications
You must be signed in to change notification settings - Fork 304
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
core
: omit invalid resource version parameters when doing paged requests
#1281
Conversation
43488be
to
788cdaf
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #1281 +/- ##
==========================================
+ Coverage 72.66% 72.78% +0.12%
==========================================
Files 75 75
Lines 6186 6196 +10
==========================================
+ Hits 4495 4510 +15
+ Misses 1691 1686 -5
|
core
: omit invalid resource version parameters when doing paged requests
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. Happy to merge this now, but leaving this for a few days in case of comments (also some overlap here with the watchlist pr).
Signed-off-by: goenning <me@goenning.net>
64d78f6
to
1d73dd1
Compare
Signed-off-by: goenning <me@goenning.net>
cc1dabe
to
ad12665
Compare
// When there's a continue token, we don't want to set resourceVersion | ||
if let Some(rv) = &self.resource_version { | ||
if rv != "0" || (rv == "0" && self.limit.is_none()) { | ||
qp.append_pair("resourceVersion", rv.as_str()); |
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.
Going over this again just as a sanity; is this possibly slightly imprecise?
If resourceVersion
is "0"
with no limit, we end up NOT serialising resourceVersion
which puts us in "Most Recent" semantics rather than "Any" semantics. Presumably this was not intended and the resourceVersion
should be serialized above the first if?
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.
Maybe we need to add a test for that too, but as per the snippet above, if rv=0 and the limit is unset, the resourceVersion is serialized.
Motivation
Fix #1278
Solution
This is inspired by what client-go does, which means we don't set the
resourceVersionMatch
andresourceVersion
when thecontinue
param is defined.I'm not sure if this covers all the scenarios, but it fixed the bug I was seeing.
As an example, if you change the
pod_paged
example to include.any_semantic()
, the response will not be paged. With this PR the response would be paged as expected.And as per your suggestion, I didn't implement this during the watcher paging loop because it's not exclusive to the watcher functionality, anyone paging with
list
would have this issue.