Arnavion / k8s-openapi

Rust definitions of the resource types in the Kubernetes client API
Apache License 2.0
379 stars 41 forks source link

Expose resource names #88

Closed MikailBag closed 3 years ago

MikailBag commented 3 years ago

Closes #73

TODO:

Arnavion commented 3 years ago

Please call it PLURAL or PLURAL_NAME rather than NAME.

MikailBag commented 3 years ago

I have renamed it to be a PLURAL_NAME

MikailBag commented 3 years ago

Last two commits additionally expose resource scope (namespace-scoped or cluster-scoped)

MikailBag commented 3 years ago

The most technically challenging part here (except parsing URLs :smile: ) is to provide enough information for type-safe cluster-scoped and namespaces resources distinction (e.g. kube-rs would like to statically prevent Api<Node>::namespaced).

In this PR, I propose roughly the following:

trait Scope {}
impl Scope for String {}
impl Scope for () {}

trait Resource {
    // ...
   type Scope: Scope;
}

// generated: 
// cluster-scoped resource example
impl Resource for Node {
   // ...
   type Scope = ();
}
// namespace resource example
impl Resource for Pod {
    // ...
   type Scope = String;
}

These type-level markers can then be used as type bounds in generic implementations.

However, the current state is not ideal, because the following code is rejected by the compiler:

struct Api<R>(std::marker::PhantomData<R>);

impl<R: Resource<Scope=String>> Api<R> {
    fn new(ns: &str) -> Self {
        todo!()
    }
}

impl<R: Resource<Scope=()>> Api<R> {
    fn new() -> Self {
        todo!()
    }
}

with an error duplicate definitions with name "new". It seems like a compiler limitation, and I can't think of a workaround. However, I think that even in the current state this PR exposes enough additional information to make generic API clients more correct and safe.

cc @clux

Arnavion commented 3 years ago

Yes, I went over this in https://github.com/Arnavion/k8s-openapi/issues/65#issuecomment-601839119

Arnavion commented 3 years ago

Just checking, this isn't ready for review yet, yes? I say this based on the unchecked TODO in the OP.

MikailBag commented 3 years ago

I think it can be reviewed.

I'd like to hear your opinion regarding those empty strings (e.g. this PR assigns empty resource name for every List, which is not really invalid, but can be confusing)

Arnavion commented 3 years ago

Is there a reason to use String and () ? If not, it should create new ClusterScopedResource and NamespaceScopedResource types and use those, like what I wrote in the second bullet-point in https://github.com/Arnavion/k8s-openapi/issues/65#issuecomment-601839119

  • instead adding const SCOPE: ResourceScope to Resource with trait ResourceScope { } struct ClusterScopedResource; impl ResourceScope for ClusterScopedResource { } struct NamespaceScopedResource; impl ResourceScope for NamespaceScopedResource { } struct NotScopedResource; impl ResourceScope for NotScopedResource { }

Though since consts can't be used for trait bounds, it should be type Scope: ResourceScope; like your current implementation.

Arnavion commented 3 years ago

As for List's PLURAL_NAME it depends on what PLURAL_NAME would be used for (by you or anyone else)? If it's just for constructing CRUD URLs dynamically, it should be fine because such URLs don't exist for lists.