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

Add WatchStreamExt::reflect to allow chaining on a reflector #1252

Merged
merged 4 commits into from
Jul 17, 2023

Conversation

clux
Copy link
Member

@clux clux commented Jul 14, 2023

Thought this might be nicer than the separate function call now that we allow doing everything on the stream chain. It also makes it clearer to see what parts get reflected (particularly pre or post modify calls) because it follows from the chain order:

watcher(api, watcher::Config::default().any_semantic())
    .default_backoff()
    .modify(|pod| { pod.managed_fields_mut().clear(); })
    .reflect(writer)
    .applied_objects();

NB: I did some rename experiments on this branch but it was a bad idea and force pushed it away. Sorry about the noise.

@codecov
Copy link

codecov bot commented Jul 14, 2023

Codecov Report

Merging #1252 (562b6c1) into main (6c9a531) will increase coverage by 0.13%.
The diff coverage is 97.05%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1252      +/-   ##
==========================================
+ Coverage   73.05%   73.19%   +0.13%     
==========================================
  Files          74       75       +1     
  Lines        5991     6025      +34     
==========================================
+ Hits         4377     4410      +33     
- Misses       1614     1615       +1     
Impacted Files Coverage Δ
kube-runtime/src/controller/mod.rs 35.15% <ø> (ø)
kube-runtime/src/utils/mod.rs 62.96% <ø> (ø)
kube-runtime/src/utils/watch_ext.rs 11.76% <0.00%> (-0.74%) ⬇️
kube-runtime/src/utils/reflect.rs 100.00% <100.00%> (ø)

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux added the changelog-add changelog added category for prs label Jul 14, 2023
@clux clux added this to the 0.85.0 milestone Jul 14, 2023
@clux clux requested review from Dav1dde and nightkr July 15, 2023 15:03
Copy link
Member

@Dav1dde Dav1dde left a comment

Choose a reason for hiding this comment

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

Just some small nits.

Should the kube::runtime::reflector be removed in favor of this?

kube-runtime/src/utils/reflect.rs Outdated Show resolved Hide resolved
kube-runtime/src/utils/reflect.rs Outdated Show resolved Hide resolved
kube-runtime/src/utils/watch_ext.rs Outdated Show resolved Hide resolved
clux and others added 3 commits July 16, 2023 22:14
Co-authored-by: David Herberth <github@dav1d.de>
Signed-off-by: Eirik A <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux
Copy link
Member Author

clux commented Jul 16, 2023

Just some small nits.

Appreciate it!

Should the kube::runtime::reflector be removed in favor of this?

Maybe. I wouldn't want to remove the fn immediately on the introduction here, but if this doesn't have any downsides, then we can probably put a longer lasting deprecation marker on reflector() in the future. For now have only tried to make some reasonable set of docs on the new method and changed over some internal usage.

@clux clux merged commit 7e8b509 into main Jul 17, 2023
17 checks passed
@clux clux deleted the watcher-reflect branch July 17, 2023 11:17
@clux clux added the runtime controller runtime related label Jul 21, 2023
@clux clux linked an issue Sep 13, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-add changelog added category for prs runtime controller runtime related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add reflector wrapper to WatchStreamExt::reflect
2 participants