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 shortnames field in kube::discovery::ApiResource #1002

Open
imuxin opened this issue Sep 8, 2022 · 7 comments · May be fixed by #1038
Open

Add shortnames field in kube::discovery::ApiResource #1002

imuxin opened this issue Sep 8, 2022 · 7 comments · May be fixed by #1038
Assignees
Labels
core generic apimachinery style work

Comments

@imuxin
Copy link
Contributor

imuxin commented Sep 8, 2022

Would you like to work on this feature?

No response

What problem are you trying to solve?

Use shortnames like kubectl request

Describe the solution you'd like

support shortname in resolve_api_resource function

fn resolve_api_resource(
    discovery: &Discovery,
    name: &str,
) -> Option<(ApiResource, ApiCapabilities)> {
    // iterate through groups to find matching kind/plural names at recommended versions
    // and then take the minimal match by group.name (equivalent to sorting groups by group.name).
    // this is equivalent to kubectl's api group preference
    discovery
        .groups()
        .flat_map(|group| {
            group
                .recommended_resources()
                .into_iter()
                .map(move |res| (group, res))
        })
        .filter(|(_, (res, _))| {
            // match on both resource name and kind name
            // ideally we should allow shortname matches as well
            name.eq_ignore_ascii_case(&res.kind) || name.eq_ignore_ascii_case(&res.plural)
        })
        .min_by_key(|(group, _res)| group.name())
        .map(|(_, res)| res)
}

Describe alternatives you've considered

Not yet.

Documentation, Adoption, Migration Strategy

No response

Target crate for feature

kube-core

@clux clux added core generic apimachinery style work help wanted Not immediately prioritised, please help! good first issue Good for newcomers labels Sep 8, 2022
@clux
Copy link
Member

clux commented Sep 8, 2022

Thanks for the issue! This is an oversight on our end. It should have been added when we made ApiResource. It exists on the native type APIResource so shouldn't be too hard to populate it from discovery where it's made in parse_apiresource.

@jmintb
Copy link
Contributor

jmintb commented Sep 18, 2022

If possible I would like to work on this. From a quick glance it looks like I need to:

  1. Add a short_names field to ApiResource.
  2. Update the code using ApiResource.
  3. Feed the short names to the ApiResource in parse_apiresource.

Does that sound about right?

@MikailBag
Copy link
Contributor

MikailBag commented Sep 18, 2022

I think shortname should go into ApiCapabilities, because:

  1. ApiResource can be constructed bypassing API discovery, e.g. using ApiResource::erase, and in those functions you won't have enough information to fill in short_names.
  2. shortname is not necessary for talking to the k8s API.

On the other hand, ApiCapabilities was initially designed exactly for all those discovered things which are not that essential (like resource scope or supported verbs)

@jmintb
Copy link
Contributor

jmintb commented Sep 21, 2022

Interesting, I think it makes more sense to include short names as an Option<Vec<String>> in ApiResource mirroring what APIResource does.

I don't think short names belong in ApiCapabilties since a short name does not describe an API capability, but rather the API resource itself, at least to me.

I am of course not intimately familiar with the project so feel free to correct me. :)

@clux
Copy link
Member

clux commented Sep 28, 2022

Slow response on this, but the question on where to put this was not immediately obvious. Have written a larger thing in #1036 on direction for these two structs. I'll wait a little bit to see what others say about it, but my vote is to put this as an Option<Vec<String>> inside ApiResource and default construct it in the places where it does not fall out of discovery (and then later merge the remaining fields + methods of ApiCapabilities into ApiResource.

@clux
Copy link
Member

clux commented Oct 1, 2022

Because of how close this was and how much restructuring was required, I ended up doing the two lines necessary on top of it in #1038 to make sure the abstraction was sound. Going to unmark as help wanted. Sorry.

@clux clux removed help wanted Not immediately prioritised, please help! good first issue Good for newcomers labels Oct 1, 2022
@clux clux self-assigned this Oct 1, 2022
@clux clux linked a pull request Oct 1, 2022 that will close this issue
@jmintb
Copy link
Contributor

jmintb commented Oct 1, 2022

No worries, that makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core generic apimachinery style work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants