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

Convert impl Condition return signature to Option<bool> #1498

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

clux
Copy link
Member

@clux clux commented May 22, 2024

To allow taking advantage of ? on Option to allow simpler Condition impls.

    fn is_pod_running() -> impl Condition<Pod> {
        |obj: Option<&Pod>| Some(obj?.status.as_ref()?.phase.as_ref()? == "Running")
    }

This also technically allows for distinguishing between;

  • condition definitively does not match the condition
  • condition could not check properties on the object

Generally, this is probably not hugely valuable, but it allows us to have a clearer answer rather than to conflate missing with either true or false.

Breaking

This is generally not a breaking change. The matches_object fn retains its original signature + behavior. It does this by using the new trait method matches with unwrap_or_default:

    fn matches_object(&self, obj: Option<&K>) -> bool {
        self.matches(obj).unwrap_or_default()
    }

However;

This is a breaking change only for implementors of Condition who now need to return an Option<bool> rather than bool for impl Condition. My thinking is that being able to use option-try makes this worth it. It can be easily communicated that you take the value and put it in a Some otherwise.

This is also a slight breaking change for the newly introduced (#1710) conditions::is_service_loadbalancer_provisioned which was returning true for services the condition did not apply for. Now it returns Some(false) for such services.

Takes advantage of `?` on `Option` to allow simpler `Condition` impls.

Not a huge change internally, but it is a breaking change.

Signed-off-by: clux <[email protected]>
@clux clux added the question Direction unclear; possibly a bug, possibly could be improved. label May 22, 2024
@clux clux changed the title Try defining runtime Condition in terms of option functions experiment defining runtime Condition in terms of option functions May 22, 2024
Copy link
Member

@Danil-Grigorev Danil-Grigorev left a comment

Choose a reason for hiding this comment

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

Lookup of the conditions is one of the most common scenarios in controller code. If anything can make it simpler, I think it is worth implementing.

@clux clux changed the title experiment defining runtime Condition in terms of option functions Convert Condition to fns returning Option<bool> Mar 12, 2025
@clux clux added the changelog-change changelog change category for prs label Mar 12, 2025
clux added 2 commits March 12, 2025 23:48
less breaking for people using it because the signature is the same!

Signed-off-by: clux <[email protected]>
Copy link

codecov bot commented Mar 13, 2025

Codecov Report

Attention: Patch coverage is 89.74359% with 4 lines in your changes missing coverage. Please review.

Project coverage is 76.0%. Comparing base (57a5e14) to head (e3b341c).

Files with missing lines Patch % Lines
kube-runtime/src/wait.rs 88.9% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main   #1498     +/-   ##
=======================================
- Coverage   76.1%   76.0%   -0.0%     
=======================================
  Files         84      84             
  Lines       7859    7843     -16     
=======================================
- Hits        5976    5960     -16     
  Misses      1883    1883             
Files with missing lines Coverage Δ
kube/src/lib.rs 89.0% <100.0%> (+0.5%) ⬆️
kube-runtime/src/wait.rs 92.1% <88.9%> (-1.3%) ⬇️
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: clux <[email protected]>
@clux clux marked this pull request as ready for review March 13, 2025 00:16
@clux
Copy link
Member Author

clux commented Mar 13, 2025

I have revamped this after seeing new verbose Condition impls, and thinking this is probably worth it with some care. Now doing it with quite minimal set of breaking changes. Have updated the PR body with details.

@clux clux changed the title Convert Condition to fns returning Option<bool> Convert impl Condition to return signature to Option<bool> Mar 13, 2025
@clux clux added runtime controller runtime related and removed question Direction unclear; possibly a bug, possibly could be improved. labels Mar 13, 2025
@clux clux changed the title Convert impl Condition to return signature to Option<bool> Convert impl Condition return signature to Option<bool> Mar 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-change changelog change category for prs runtime controller runtime related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants