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::modify() to modify events #1246

Merged
merged 1 commit into from
Jul 11, 2023

Conversation

aryan9600
Copy link
Contributor

Motivation

Atm, we need to resort to nested maps if we want to modify the resource directly present in the Events returned by the watcher() stream.

let stream = watcher(api, watcher::Config::default()).map_ok(|ev| {
    ev.modify(|pod| {
        pod.managed_fields_mut().clear();
        pod.annotations_mut().clear();
        pod.status = None;
    })
});

Solution

Add WatchStreamExt::modify() which returns an EventModify stream. This allows for users to directly modify the inner value of an Event returned by the watcher stream, thus avoiding the usage of nested maps.

Example usage:

let stream = watcher(api, watcher::Config::default()).modify(|pod| {
    pod.managed_fields_mut().clear();
    pod.annotations_mut().clear();
    pod.status = None;
});

Fixes #1245

@codecov
Copy link

codecov bot commented Jul 8, 2023

Codecov Report

Merging #1246 (85065c9) into main (db585dd) will increase coverage by 0.22%.
The diff coverage is 96.15%.

❗ Current head 85065c9 differs from pull request most recent head a58f446. Consider uploading reports for the commit a58f446 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1246      +/-   ##
==========================================
+ Coverage   72.60%   72.82%   +0.22%     
==========================================
  Files          72       73       +1     
  Lines        5899     5925      +26     
==========================================
+ Hits         4283     4315      +32     
+ Misses       1616     1610       -6     
Impacted Files Coverage Δ
kube-runtime/src/utils/mod.rs 64.15% <ø> (ø)
kube-runtime/src/utils/watch_ext.rs 0.00% <0.00%> (ø)
kube-runtime/src/utils/event_modify.rs 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

@clux clux self-assigned this Jul 8, 2023
@clux clux added the changelog-add changelog added category for prs label Jul 8, 2023
@clux clux added this to the 0.84.0 milestone Jul 8, 2023
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

Great work. Thanks a lot for this. One minor test request + have inlined a doc example after verifying locally.

kube-runtime/src/utils/watch_ext.rs Outdated Show resolved Hide resolved
kube-runtime/src/utils/event_modify.rs Outdated Show resolved Hide resolved
@aryan9600 aryan9600 force-pushed the event-modify branch 2 times, most recently from 6ca8651 to 85065c9 Compare July 8, 2023 12:12
Copy link
Member

@clux clux left a comment

Choose a reason for hiding this comment

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

Looks great. Tyvm!

Copy link
Member

@nightkr nightkr left a comment

Choose a reason for hiding this comment

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

Largely looks good to me! One more serious concern, but that's more about the existing Event::modify than about this PR.

kube-runtime/src/utils/event_modify.rs Outdated Show resolved Hide resolved
Comment on lines 76 to 81
assert!(matches!(
poll!(ev_modify.next()),
Poll::Ready(Some(Ok(Event::Restarted(restart_val))))
));
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a bug in Event::modify if this passes, this should be vec![11].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its not a bug in Event::modify, its a shortcoming of matches!. this will always pass, since its not actually using restart_val (the compiler also complains about restart_val being unused) (ref: https://stackoverflow.com/questions/68155018/rust-matches-macro-behaves-unexpectedly-in-regards-to-enum)

fixed it by extracting the result and using an if guard to compare instead of directly specifying the desired value.

Comment on lines +68 to +70
assert!(matches!(
poll!(ev_modify.next()),
Poll::Ready(Some(Ok(Event::Applied(1))))
));
Copy link
Member

Choose a reason for hiding this comment

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

Nit: No need to use poll! if you expect it to be Ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as far as i understand, the poll! here is used to get convert the future returned by ev_modify.next() into a Poll object.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but since it should already be ready it would be equivalent to ev_modify.next().await. Then again, on second thought, poll! does have the advantage of failing faster if it's not ready for whatever reason.

Add `WatchStreamExt::modify()` which returns an `EventModify` stream.
This allows for users to directly modify the inner value of an `Event`
returned by the watcher stream, thus avoiding the usage of nested maps.

Example usage:

```rust
let stream = watcher(api, watcher::Config::default()).modify(|pod| {
    pod.managed_fields_mut().clear();
    pod.annotations_mut().clear();
    pod.status = None;
});
```

Signed-off-by: Sanskar Jaiswal <[email protected]>
Copy link
Member

@nightkr nightkr left a comment

Choose a reason for hiding this comment

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

LGTM

That said, please try to avoid rebasing/squashing feature branches if possible, it makes reviews a pain.

@clux clux merged commit e99d0dc into kube-rs:main Jul 11, 2023
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a WatchStreamExt::modify shortcut
4 participants