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

Genericize PartialObjectMeta over the underlying Resource #1152

Merged
merged 9 commits into from
Mar 2, 2023
Merged

Conversation

clux
Copy link
Member

@clux clux commented Feb 25, 2023

Allows doing static dispatch using the root type, and avoids having to do stringly typed oob signalling in generic code.

Fixes #1151, in particular:

  • Adds a serde skipped + defaulted PhantomType<K> to PartialObjectMeta
  • Fixes broken Resource impl for PartialObjectMeta by instead proxying to K: Resource
  • I.e. all usage of PartialObjectMeta has changed to PartialObjectMeta<K>
  • Still works dynamically via PartialObjectMeta<DynamicObject> (which it is inferred to in dynamic_watcher)
  • Fixes up extension names in metadata api in core_methods used for tracing
  • Adds a converter trait PartialObjectMetaExt from ObjectMeta into PartialObjectMeta<K>

Example generic code:

pub async fn reconcile<K>(obj: Arc<PartialObjectMeta<K>>, ctx: Arc<Context>) -> Result<Action>
where
    K: Resource<Scope = NamespaceResourceScope, DynamicType = ()>
        + Clone
        + DeserializeOwned
        + Debug,
{
    let kind = K::kind(&()).to_string();
    let api: Api<PartialObjectMeta<K>> = Api::default_namespaced(ctx.client.clone());

This now gives us a way to get type info in generic code without extra ApiResource args and lambdas, while also maintaining all the static type safety we have around scopes.

Allows doing static dispatch using the root type, and avoids
having to do stringly typed oob signalling in generic code.

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux changed the title Genericize PartialObjectMeta over the underlying Resource Genericize PartialObjectMeta over the underlying Resource Feb 25, 2023
Signed-off-by: clux <sszynrae@gmail.com>
@codecov-commenter
Copy link

codecov-commenter commented Feb 25, 2023

Codecov Report

Merging #1152 (c228fb6) into main (825aba8) will increase coverage by 0.30%.
The diff coverage is 86.84%.

❗ Current head c228fb6 differs from pull request most recent head 1ecb6ec. Consider uploading reports for the commit 1ecb6ec to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1152      +/-   ##
==========================================
+ Coverage   72.26%   72.57%   +0.30%     
==========================================
  Files          67       67              
  Lines        5106     5123      +17     
==========================================
+ Hits         3690     3718      +28     
+ Misses       1416     1405      -11     
Impacted Files Coverage Δ
kube-client/src/api/mod.rs 68.75% <ø> (ø)
kube-runtime/src/watcher.rs 41.50% <ø> (ø)
kube-client/src/api/core_methods.rs 71.83% <72.72%> (ø)
kube-core/src/metadata.rs 85.71% <91.66%> (+70.32%) ⬆️
kube-client/src/lib.rs 93.95% <100.00%> (+0.06%) ⬆️
kube-core/src/resource.rs 58.33% <0.00%> (+4.76%) ⬆️

Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
@clux clux added this to the 0.80.0 milestone Feb 25, 2023
@clux clux added runtime controller runtime related core generic apimachinery style work changelog-change changelog change category for prs labels Feb 25, 2023
@clux clux marked this pull request as ready for review February 25, 2023 08:11
@clux clux requested a review from nightkr February 25, 2023 08:14
@clux
Copy link
Member Author

clux commented Feb 25, 2023

cc @mateiidavid

conversion the other way avoids users having to write:

```rust
let pom = PartialObjectMeta::<Pod> {
   types: Some(TypeMeta{ ...constant }),
   metadata: actual_test_data,
   ..Default::default(),
};
```

Signed-off-by: clux <sszynrae@gmail.com>
Signed-off-by: clux <sszynrae@gmail.com>
Copy link
Member

@nightkr nightkr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM overall. I'm starting to wonder if this would let us remove the dedicated Api::*_metadata methods and just infer that based on K, but that's probably a question for another time.

type DynamicType = ApiResource;
type Scope = DynamicResourceScope;
// Unit tests often convert the other way
impl<K> From<ObjectMeta> for PartialObjectMeta<K> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a valid thing in general, or do we want to make it a dedicated associated function where we can call out any caveats? Should we take the TypeMeta from K instead of making up our own?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh, this was a really good call. Thank you. The From is way too simplistic here because how we want to use PartialObjectMeta depends on whether it is a request object or a response object (the apiserver erases the type, but the root type is needed for patch).

Have removed the From and instead created a sealed PartialObjectMetaExt trait that contains two converter functions, and have used it in the main test case.

kube-core/src/metadata.rs Outdated Show resolved Hide resolved
kube-core/src/metadata.rs Outdated Show resolved Hide resolved
clux added 3 commits March 1, 2023 06:06
Need to have two different ways to convert from ObjectMeta depending on how we are going to use it.
Have created a sealed trait with converters for it, and updated docs, tests to use it.

Signed-off-by: clux <sszynrae@gmail.com>
avoids unstable associated trait bounds + lints showing that the default really had to be the empty type.

Signed-off-by: clux <sszynrae@gmail.com>
bunch of new bugs in https://github.com/rust-lang/rust-clippy/issues?q=is%3Aissue+let_underscore_untyped
our use seems perfectly fine, what clippy wanted was nonsense:

```
error: non-binding `let` without a type annotation
  --> kube-runtime/src/reflector/store.rs:87:12
   |
87 | pub struct Store<K: 'static + Resource>
   |            ^^^^^
   |
   = help: consider adding a type annotation or removing the `let` keyword
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_untyped

error: non-binding `let` without a type annotation
   --> kube-runtime/src/watcher.rs:123:5
    |
123 |     InitListed { resource_version: String },
    |     ^^^^^^^^^^
    |
    = help: consider adding a type annotation or removing the `let` keyword
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#let_underscore_untyped
```

Signed-off-by: clux <sszynrae@gmail.com>
@clux clux merged commit f4dec45 into main Mar 2, 2023
@clux clux deleted the meta-genericize branch March 2, 2023 03:38
@clux clux mentioned this pull request Mar 15, 2023
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-change changelog change category for prs core generic apimachinery style work runtime controller runtime related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Genericize PartialObjectMeta over the underlying kind
3 participants