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 GetParams to mirror GetOptional #1174

Closed
clux opened this issue Mar 30, 2023 · 2 comments · Fixed by #1214
Closed

Add GetParams to mirror GetOptional #1174

clux opened this issue Mar 30, 2023 · 2 comments · Fixed by #1214
Labels
core generic apimachinery style work help wanted Not immediately prioritised, please help!

Comments

@clux
Copy link
Member

clux commented Mar 30, 2023

What problem are you trying to solve?

Allow querying Api::get with Any, NotOlderThan and MostRecent semantics

https://kubernetes.io/docs/reference/using-api/api-concepts/#semantics-for-get-and-list
https://pkg.go.dev/k8s.io/apimachinery/pkg/apis/meta/v1#GetOptions

Stashing this in an issue as we are waiting for #1171 first.

Describe the solution you'd like

Something similar to what was recently added to ListParams, but we cannot use the VersionMatch enum here as that is not a query parameter for get calls.

/// Common query parameters used in get calls
#[derive(Clone, Debug, Default)]
pub struct GetParams {
    /// An explicit resource version with implicit version match strategies
    ///
    /// Default unset gives the most recent
    /// "0" gives the less consistent but-cheaper "Any", and an explicit value is like `VersionMatch::NotOlderThan`.
    /// See <https://kubernetes.io/docs/reference/using-api/api-concepts/#resource-versions> for details.
    pub resource_version: Option<String>,
}

and then possibly implement some helpers to make the new semantics more easy to setup:

impl GetParams {
    pub fn at(resource_version: &str) -> Self {
        Self { resource_version: Some(resource_version.into()) } 
    }
    pub fn any() -> Self {
        Self::at("0")
    }
}

this can then be added to Request::get:

    pub fn get(&self, name: &str, gp: &GetParams) -> Result<http::Request<Vec<u8>>, Error> {
        let target = format!("{}/{}", self.url_path, name);
        let mut qp = form_urlencoded::Serializer::new(target);
        if let Some(rv) = &gp.resource_version {
            qp.append_pair("resourceVersion", rv.as_str());
        }
        let urlstr = qp.finish();
        let req = http::Request::get(urlstr);
        req.body(vec![]).map_err(Error::BuildRequest)
    }

Api interface

We already have Api::get without GetParams, and if that was the only interface it'd be easy to either add to that, or add another get variant that takes the new params. However we also have Api::get_opt and Api::entry. So not quite sure what to do about those. Don't really want to expose variant methods for all those, so it's probably better to change the signatures of these 3.

Documentation, Adoption, Migration Strategy

Release highlight if doing breaking change.

Target crate for feature

kube-core, kube-client

@clux clux added the core generic apimachinery style work label Mar 30, 2023
@nabokihms
Copy link
Contributor

Honestly, thought about adding this next to list options 😅

@clux clux added the help wanted Not immediately prioritised, please help! label Apr 7, 2023
@mateiidavid
Copy link
Contributor

Thanks for a super thorough description and suggestion :) I can take this.

mateiidavid added a commit to mateiidavid/kube that referenced this issue May 2, 2023
As part of a larger change to support version matching strategies (to
reduce I/O pressure), ListParams were separated into List and Watch
params. To complete the feature set, this change adds support for
`GetParams`, mirroing the upstream client-go layout for the type.

In the context of this change, we added a GetParams struct that does
implicit version matching (semantics are quite easy, any = "0",
NotOlderThan = specific, non-zero version, and unspecified is most
recent). We use the struct in all request methods and in Api core
methods. `ListParams` with version matching semantics is used for
list_metadata, so the same was done here (extended GetParams to
get_metadata and all other asssociated methods) even if it was outside
of the scope of the issue.

As part of the change, we also had to change the examples and tests.
Since the version was (largely) unspecified, we use the default derived
implementation; for tests and examples, this change should be
non-functional (unspecified is the same as unset).

This is a breaking change.

Fixes kube-rs#1174

Signed-off-by: Matei David <[email protected]>
mateiidavid added a commit to mateiidavid/kube that referenced this issue May 3, 2023
As part of a larger change to support version matching strategies (to
reduce I/O pressure), ListParams were separated into List and Watch
params. To complete the feature set, this change adds support for
`GetParams`, mirroing the upstream client-go layout for the type.

In the context of this change, we added a GetParams struct that does
implicit version matching (semantics are quite easy, any = "0",
NotOlderThan = specific, non-zero version, and unspecified is most
recent). We use the struct in all request methods and in Api core
methods. `ListParams` with version matching semantics is used for
list_metadata, so the same was done here (extended GetParams to
get_metadata and all other asssociated methods) even if it was outside
of the scope of the issue.

As part of the change, we also had to change the examples and tests.
Since the version was (largely) unspecified, we use the default derived
implementation; for tests and examples, this change should be
non-functional (unspecified is the same as unset).

Fixes kube-rs#1174

Signed-off-by: Matei David <[email protected]>
@clux clux closed this as completed in #1214 May 5, 2023
clux pushed a commit that referenced this issue May 5, 2023
* Introduce `GetParams` support

As part of a larger change to support version matching strategies (to
reduce I/O pressure), ListParams were separated into List and Watch
params. To complete the feature set, this change adds support for
`GetParams`, mirroing the upstream client-go layout for the type.

In the context of this change, we added a GetParams struct that does
implicit version matching (semantics are quite easy, any = "0",
NotOlderThan = specific, non-zero version, and unspecified is most
recent). We use the struct in all request methods and in Api core
methods. `ListParams` with version matching semantics is used for
list_metadata, so the same was done here (extended GetParams to
get_metadata and all other asssociated methods) even if it was outside
of the scope of the issue.

As part of the change, we also had to change the examples and tests.
Since the version was (largely) unspecified, we use the default derived
implementation; for tests and examples, this change should be
non-functional (unspecified is the same as unset).

Fixes #1174

Signed-off-by: Matei David <[email protected]>

* Correctly quote resourceVersion in code docs

Signed-off-by: Matei David <[email protected]>

* Add unit tests

Signed-off-by: Matei David <[email protected]>

---------

Signed-off-by: Matei David <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core generic apimachinery style work help wanted Not immediately prioritised, please help!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants