More ergonomic ListParams::labels

[tomkarw]

Would you like to work on this feature?

yes

What problem are you trying to solve?

If I run:

use kube::api::ListParams;

fn main() {
    let list_params = ListParams::default().labels("a=1").labels("b=2");
    println!("labels: {:?}", list_params.label_selector);
}

I'd expect:

labels: Some("a=1, b=2")

but I'm getting:

labels: Some("b=2")

That's because ListParams::labels overrides previously set values, which IMO is unexpected behavior in a Builder Pattern.

Describe the solution you'd like

pub fn labels(mut self, label_selector: &str) -> Self {
    if let Some(current_label_selector) = self.label_selector.as_mut() {
        current_label_selector.push(',');
        current_label_selector.push_str(label_selector);
    } else {
        self.label_selector = Some(label_selector.to_string());
    }
    self
}

Same would apply to ListParams::fields and WatchList::{lables,fields}.

This has a caveat of being a "silent" Breaking Change, as the behavior will change, but the API will stay the same.

Describe alternatives you've considered

Introducing a new method which allows chaining, under a different name.

More-or-less a pseudocode:

pub fn label(mut self, label_selector: &str) -> Result<Self> {
    // maybe validate label? but it would mean returning an error
    if !label_selector.contains('=') || label_selector.contains(',') || label_selector.contains(' ') {
        return Err("invalid label");
    }

    if let Some(current_label_selector) = self.label_selector.as_mut() {
        current_label_selector.push(',');
        current_label_selector.push_str(label_selector);
    } else {
        self.label_selector = Some(label_selector.to_string());
    }

    Ok(self)
}

Documentation, Adoption, Migration Strategy

No response

Target crate for feature

kube-core

[clux]

Label Selector helper wrappers is something I've kind of wanted for a while myself. Personally I am not sure about a mutably borrowable string here, because that feels equally error-prone.

Have you seen what linkerd is doing with actually typed label selector expressions?
We could possibly expose something like that - which could then be inserted into ListParams when it's fully built - that possibly feels more optimal.

[SOF3]

imo it sounds reasonable to override the field completely in a builder pattern, since that would be the case for other non-collection fields as well. Maybe you want a .label(k, v)?