Add support for CRD ConversionReview types

[MikailBag]

Signed-off-by: Mikail Bagishov [email protected]

Motivation

Part of #992.

Solution

Compared to version in the old PR, I renamed low_level.rs -> types.rs, renamed module conversion.rs -> mod/conversion.rs to align with rest of the code base, and made submodule private.

[codecov-commenter]

Codecov Report

Merging #999 (f03ec56) into master (15faafb) will decrease coverage by 0.30%.
The diff coverage is 49.25%.

@@            Coverage Diff             @@
##           master     #999      +/-   ##
==========================================
- Coverage   72.20%   71.89%   -0.31%     
==========================================
  Files          64       65       +1     
  Lines        4573     4637      +64     
==========================================
+ Hits         3302     3334      +32     
- Misses       1271     1303      +32     

Impacted Files	Coverage Δ
kube-core/src/admission.rs	61.11% <0.00%> (ø)
kube-core/src/response.rs	31.03% <9.09%> (-68.97%)	⬇️
kube-core/src/conversion/types.rs	71.42% <71.42%> (ø)
kube-client/src/lib.rs	93.75% <100.00%> (ø)

[clux]

minor comments on documentation and naming, but this part is starting to look good

[clux]

In admission we have this as a non-optional field for both Response and Request (with skip still), is there a reason to keep it optional?

[MikailBag]

No strong reason. It simplifies manual creation of ConversionRequest a little bit (i.e. it may be surprising for user to see non-optional field which is not set by apiserver).

[clux]

maybe we should split this up into a failure with message as the only param and have a with_reason(reason) function on mut self (or other way around if reason is the most important one - don't remember right now)

[clux]

actually, these two are very linked, it is probably a mistake to let people do one and not the other. ignore this.

while i am a bit uncomfortable with the double &str argument (which can be swapped around accidentally), there are probably smarter stuff we can do down the line to convert an Error into a Status by taking advantage of the Display and Debug traits for the error to fill these in a less error prone way.

[MikailBag]

I can think of

fn from_error<E: Display>(err: &E, reason: &str) -> Self;

which uses e.to_string() as message (I don't see a way to infer reason from error, especially given guidelines that it shoud be short and PascalCase).

[clux]

Users could possibly do it in their app, if they did a convention like that if the Display was always PascalCase, and then they can take the Debug as the reason, but users would have to stay on top of their errors if so. We couldn't enforce that though, so there isn't much we can do in kube directly. We can experiment in controller-rs with some strategic error use in the future perhaps.

[clux]

Only one tiny nit left on the admission code that was cleaned up, but am otherwise happy with merging this. Thanks for all the back and forth on this.

[MikailBag]

CI is red, but this looks like issue with k3d action

[clux]

CI is red, but this looks like issue with k3d action

Yeah, had the same issue on the k8s-openapi bump PR, so probably not resolvable here without pinning k3d. I'll give it one last re-run as a check before merging.

[clux]

Yeah, consistently k3d action failing. I'll fix that in master with some pinning if necessary. Going to deal with this anyway in #1008.

Going to merge this. Thanks a lot for all the work on this!