Skip to content
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

Fix WatchParams bookmarks for watch_metadata #1193

Merged
merged 3 commits into from Apr 12, 2023
Merged

Fix WatchParams bookmarks for watch_metadata #1193

merged 3 commits into from Apr 12, 2023

Conversation

clux
Copy link
Member

@clux clux commented Apr 8, 2023

Intended to factor out populate_qp from ListParams and do the same in WatchParams, but accidentally found a place where the duplication caused a split.

This brings the query param serialization in line for all variants of list, and does the same for all variants of watch (where the inconsistency was wound).

Intended to factor out `populate_qp` from `ListParams` and do the same in `WatchParams`,
but accidentally found a place where the duplication caused a split.

This brings the query param serialization in line for all variants of list,
and does the same for all variants of watch (where the inconsistency was wound).

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux added the changelog-fix changelog fix category for prs label Apr 8, 2023
@clux clux added this to the 0.83.0 milestone Apr 8, 2023
@codecov
Copy link

codecov bot commented Apr 8, 2023

Codecov Report

Merging #1193 (050426a) into main (6f3cfac) will increase coverage by 0.08%.
The diff coverage is 89.28%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1193      +/-   ##
==========================================
+ Coverage   73.42%   73.51%   +0.08%     
==========================================
  Files          68       68              
  Lines        5370     5357      -13     
==========================================
- Hits         3943     3938       -5     
+ Misses       1427     1419       -8     
Impacted Files Coverage Δ
kube-core/src/admission.rs 61.11% <ø> (ø)
kube-core/src/params.rs 80.30% <86.95%> (+0.87%) ⬆️
kube-core/src/request.rs 98.63% <100.00%> (+2.35%) ⬆️

... and 1 file with indirect coverage changes

@clux clux requested a review from mateiidavid April 8, 2023 16:40
@clux
Copy link
Member Author

clux commented Apr 8, 2023

@mateiidavid just checking in to make sure was if there was real reason to exclude watch bookmarks from the metadata watches.

@clux clux marked this pull request as ready for review April 8, 2023 16:41
@mateiidavid
Copy link
Contributor

mateiidavid commented Apr 11, 2023

@clux

@mateiidavid just checking in to make sure was if there was real reason to exclude watch bookmarks from the metadata watches.

In all honesty, it might have just been me being short-sighted. There isn't afaik a real reason to exclude them.

Copy link
Contributor

@mateiidavid mateiidavid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be missing some context here, tried to get up to speed with the params split change. I'd urge another reviewer to check this out 😬

As far as metadata watches go, they should be ok with bookmarks, went back to my PR and I seemed to have manually validated that it works using curl

:; curl -H "Accept: application/json;as=PartialObjectMetadata;g=meta.k8s.io;v=v1" "http://localhost:8001/api/v1/namespaces/kube-system/pods?&w
atch=true&resourceVersion=0&timeoutSeconds=290&allowWatchBookmarks=true"
{"type":"ADDED","object":{"kind":"PartialObjectMetadata","apiVersion":"meta.k8s.io/v1","metadata":{"name":"local-path-provisioner-7b7dc8d6f5-ckv7f","generateName":"local-path-provisioner-7b7dc8d6f5-","namespace":"kube-system","uid":"aee9cd50-1d63-42ae-92c7-de43ca7ba3a1","resourceVersion":"473","creationTimestamp":"2023-04-05T11:23:40Z","labels":{"app":"local-path-provisioner","pod-template-hash":"7b7dc8d6f5"},"ownerReferences":[{"apiVersion":"apps/v1","kind":"ReplicaSet","name":"local-path-provisioner-7b7dc8d6f5","uid":"5fbe213e-302a-4c97-b33a-0f29d2645bea","controller":true,"blockOwnerDeletion":true}],"managedFields":[{"manager":"k3s","operation":"Update","apiVersion":"v1","time":"2023-04-05T11:23:40Z","fieldsType":"FieldsV1","fieldsV1":{"f:metadata":{"f:generateName":{},"f:labels":{".":{},"f:app":{},"f:pod-template-hash":{}},"f:ownerReferences":{".":{},"k:{\"uid\":\"5fbe213e-302a-4c97-b33a-0f29d2645bea\"}":{}}},"f:spec":{"f:containers":{"k:{\"name\":\"local-path-provisioner\"}":{".":{},"f:command":{},"f:env":{".":{},"k:{\"name\":\"POD_NAMESPACE\"}":{".":{},"f:name":{},"f:valueFrom":{".":{},"f:fieldRef":{}}}},"f:image":{},"f:imagePullPolicy":{},"f:name":{},"f:resources":{},"f:terminationMessagePath":{},"f:terminationMessagePolicy":{},"f:volumeMounts":{".":{},"k:{\"mountPath\":\"/etc/config/\"}":{".":{},"f:mountPath":{},"f:name":{}}}}},"f:dnsPolicy":{},"f:enableServiceLinks":{},"f:priorityClassName":{},"f:restartPolicy":{},"f:schedulerName":{},"f:securityContext":{},"f:serviceAccount":{},"f:serviceAccountName":{},"f:terminationGracePeriodSeconds":{},"f:tolerations":{},"f:volumes":{".":{},"k:{\"name\":\"config-volume\"}":{".":{},"f:configMap":{".":{},"f:defaultMode":{},"f:name":{}},"f:name":{}}}}}},{"manager":"k3s","operation":"Update","apiVersion":"v1","time":"2023-04-05T11:23:46Z","fieldsType":"FieldsV1","fieldsV1":{"f:status":{"f:conditions":{"k:{\"type\":\"ContainersReady\"}":{".":{},"f:lastProbeTime":{},"f:lastTransitionTime":{},"f:status":{},"f:type":{}},"k:{\"type\":\"Initialized\"}":{".":{},"f:lastProbeTime":{},"f:lastTransitionTime":{},"f:status":{},"f:type":{}},"k:{\"type\":\"Ready\"}":{".":{},"f:lastProbeTime":{},"f:lastTransitionTime":{},"f:status":{},"f:type":{}}},"f:containerStatuses":{},"f:hostIP":{},"f:phase":{},"f:podIP":{},"f:podIPs":{".":{},"k:{\"ip\":\"10.42.0.5\"}":{".":{},"f:ip":{}}},"f:startTime":{}}},"subresource":"status"}]}}}

Left a tiny nit since I started going through the code. Think the refactor looks great.

kube-core/src/params.rs Show resolved Hide resolved
@clux
Copy link
Member Author

clux commented Apr 11, 2023

As far as metadata watches go, they should be ok with bookmarks, went back to my PR and I seemed to have manually validated that it works using curl

Thanks for checking!

@clux clux merged commit d294847 into main Apr 12, 2023
17 of 18 checks passed
@clux clux deleted the populate-qp branch April 12, 2023 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-fix changelog fix category for prs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants