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.

Sign up for GitHub

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

Jump to bottom

Should Api handle serialization too? #142

Closed
nightkr opened this issue Feb 20, 2020 · 3 comments · Fixed by #439
Closed

Should Api handle serialization too? #142

nightkr opened this issue Feb 20, 2020 · 3 comments · Fixed by #439
Assignees
@clux
Labels
api Api abstraction related

Comments

@nightkr
Copy link
Member

nightkr commented Feb 20, 2020

It feels weird that deserialization is out of your control, but requests are just untyped Vec<u8>s.

Patches would be trickier to get right, though. Hmm..
nightkr added a commit to Appva/kube-rs that referenced this issue Feb 20, 2020
@nightkr
Make Api::create and Api::replace take type-safe KubeObjects
34c94ab 
`Vec<u8>`s are still supported, but deprecated. Sadly, `#[deprecated]` doesn't
work for trait impls, so a doccomment was used instead.

Fixes kube-rs#142.
@nightkr nightkr mentioned this issue Feb 20, 2020
Closed
@clux clux added the api Api abstraction related label Feb 26, 2020
clux added a commit that referenced this issue Mar 7, 2020
@clux
separate seralization type from the api resource type - #142
07bb9e2 
lets you use Api without the generic type parameter, but all the methods
have the generic type param.

this lets you re-use a `Api::namespaced::<Pod>(...)` (say) by doing:
- api.get::<Pod>(...)
- api.get::<LimitedPod>)(...)

which can be useful sometimes.

Not entirely sure about this, but it has a use-case for me where I need
to re-define a CRD type twice (one with the api-guaranteed fields, and
one without)
@clux
Copy link
Member

clux commented Mar 8, 2020 •
edited
Loading

Api::create and Api::replace now expect typed objects that can be serialised to the specified K.

Api::patch (and proposed helpers around patch) requires extra work like what is proposed in #175 .

For the subresource Api::replace_scale.. kubernetes allows skipping out Spec, whereas #[derive(CustomResource)] does not (because it hardcodes .spec as set).

Not sure what's sensible for this.. could do one of these 3:

  • replace_scale to take a full K (including a .spec that's ultimately unused by the api)
  • replace_scale to only takes a Scale struct and we nest it internally inside a { "scale": scaledata }
  • make .spec optional in kube-derive (leading to unwraps everywhere else), but allows us to use K in replace_scale without specifying the .spec part

@nightkr
Copy link
Member Author

nightkr commented Mar 8, 2020

FWIW this applies to replace_metadata too.

I like 2. Doesn't lead to a bunch of useless unwrap()s, doesn't cause confusion around whether other values are ignored, and avoids a bunch of useless clones.

@clux clux mentioned this issue Jul 26, 2020
Open
@clux
Copy link
Member

clux commented Feb 27, 2021

Going to close this upon further reflection.

  • Don't really want to hamstring kube-derive for a special case like replace_scale.
  • Api::patch with server-side apply is now standard.

Will document patch it a little better, then close.

@clux clux self-assigned this Feb 27, 2021
clux added a commit that referenced this issue Feb 27, 2021
@clux
update patch documentation - closes #142
aee3d38
@clux clux linked a pull request Feb 27, 2021 that will close this issue
update patch documentation #439
Merged
@clux clux closed this as completed in #439 Feb 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@clux clux

Labels
api Api abstraction related
Projects
None yet
Milestone
No milestone
2 participants
@clux @nightkr