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

Merge ApiCapabilities into ApiResource #1036

Open
clux opened this issue Sep 28, 2022 · 1 comment · May be fixed by #1038
Open

Merge ApiCapabilities into ApiResource #1036

clux opened this issue Sep 28, 2022 · 1 comment · May be fixed by #1038
Assignees
Labels
core generic apimachinery style work question Direction unclear; possibly a bug, possibly could be improved.

Comments

@clux
Copy link
Member

clux commented Sep 28, 2022

Our ApiResource is intentionally a subset of the upstream APIResource that lets us be able to use the type as a Resource. It has a special meaning as the DynamicType for Resource in the dynamic discovery cases.

ApiCapabilities is more the other part of that; the parts that is not essential to have to use the type with the api, but for helpful for detecting "what you can do with it".

However, the way we have split it is becoming harder to justify:

Thus, I think we should reduce the distance between the native Kubernetes types and do one of the following:

Option 1: Bundle capabilities as an optional field inside ApiResource

A least breaking change type thing.

Option 2: Merge fields from ApiCapabilities as optional fields inside ApiResource

The most breaking potential change. This needs additional breaking changes to flatten the methods on ApiCapabilities into ApiResource.

In both cases:

  • discovery goes from returning Vec<(ApiResource, ApiCapabilities)> to Vec<ApiResource>
  • ApiResource::erase can set whatever fields we don't need in the runtime to None.

And we would get the struct more in-line with the upstream type, while retaining their light-weight use (optional types does not have an extra memory overhead when they are None) as a DynamicType.

Doing the most breaking change (full merge) I think would be easier because it also helps solve the problem of where to put the upstream fields, and help get direction on things like #1002.

@clux clux added core generic apimachinery style work question Direction unclear; possibly a bug, possibly could be improved. labels Sep 28, 2022
@clux clux changed the title Merge ApiCapabilities on ApiResource Merge ApiCapabilities into ApiResource Sep 28, 2022
@clux
Copy link
Member Author

clux commented Sep 30, 2022

Clarifying the argument for it for Scope as it came immediately in #1037.

It's not possible for the new methods to protect against scope misuse on dynamic types as it stands:

impl Client {
    async fn create_namespaced_dyn<K>(&self, ns: &Namespace, pp: &PostParams, data: &K, dt: &K::DynamicType) -> Result<K>
    where:
        K: Resource<Scope = DynamicResourceScope> + Serialize + DeserializeOwned + Clone + Debug,
    {
        // TODO: need to runtime limit function based on the dynamic Scope of the dynamic resource
    }

Because of the dynamic type, scope needs to be limited at runtime rather than by a type constraint. This currently cannot be done because the Scope resides on ApiCapabilities but we really need it inside the DynamicType (which is ApiResource).

@clux clux linked a pull request Sep 30, 2022 that will close this issue
@clux clux self-assigned this Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core generic apimachinery style work question Direction unclear; possibly a bug, possibly could be improved.
Projects
Status: Blocked
Development

Successfully merging a pull request may close this issue.

1 participant