fix(stackable-versioned): Fix bugs in ConversionReviews integration

[sbernauer]

Motivation

#1056 will also fix at least some of the bugs, but I would prefer to fix the bugs fast and have tests.
I'm happy that my code gets overwritten by #1056 as longs as the tests pass.
By fixing the problems we can continue on working an the actual conversion HTTPS webhook stuff.

Description

Fix some problems in the (brand new) conversions. And most important: Re-add tests that temporarily have been removed from #1050

• check if we know about the desired version, error if not. Previously we would noop for unknown desired versions.
• k8s sends us test.stackable.tech/v1alpha1 as apiVersion, not v1alpha1
• Check that the sent k8s kind is the one we expect

PS: The diff is so big because lots of tests (snapshots) changed. Actual code change is only a small portion.

Definition of Done Checklist

• Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
• Please make sure all these things are done and tick the boxes

Author

• Changes are OpenShift compatible
• CRD changes approved
• CRD documentation for all fields, following the style guide.
• Integration tests passed (for non trivial changes)
• Changes need to be "offline" compatible

Reviewer

• Code contains useful comments
• Code contains useful logging statements
• (Integration-)Test cases added
• Documentation added or updated. Follows the style guide.
• Changelog updated
• Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

• Feature Tracker has been updated
• Proper release label has been added

[NickLarsenNZ]

Just a couple of suggestions so far

[Techassi]

note: I think we should stick with the parameter name value here. In my mind, this can only be called an object after it has been parsed as such. Also, the function name suggests that this parameter should be called value.

[sbernauer]

I already had all of this thoughts and though about renaming the function to smh like `try_from_json_object) but didn't want to get into bikeshedding. But here we are :)

This function expects you to pass a k8s object to it, not an arbitrary JSON value such as f32 or String. I think we should make this clear in the name, I would therefore propose to name it fn try_from_json_object(object: #serde_json_path::Value) or fn try_from_json_k8s_object(k8s_object: #serde_json_path::Value) (the try_ prefix is already under discussion in #1061 (comment))

[Techassi]

I would be fine with fn try_from_json_object(object_value: #serde_json_path::Value) and internally clearly separate between the "raw" object and the successfully deserialized "object" we return in the end.

In my opinion the try_ prefix doesn't provide a whole lot here. The function clearly indicates that it is fallible by returning a Result.

[sbernauer]

That sounds like a good compromise! 119a845

[Techassi]

note: I would like to avoid this. Instead, we want automatic support for integration tests by moving the "tests" folder one layer up (out of "src").

[sbernauer]

Moved in 8f77619

[Techassi]

suggestion: I would additionally drop the fixtures part from the folder structure. Then we have the same structure as in stackable-versioned-macros.

suggestion: I would also rename the tests/integration_test.rs file to tests/conversion.rs because we are testing the conversion. There might be other kind of integration tests in the future.

[sbernauer]

works for me

7400d39 now has files such as crates/stackable-versioned/tests/inputs/conversions/fail/wrong_kind.json

[Techassi]

note: Similar to stackable-versioned-macros, I would like this function to live in a test_utils module (which is gated behind #[cfg(test)].

I suggest to adjust the name to better indicate what this function does and adding a dedicated error enum similar to expand_from_file.

Suggested change

	fn run_for_file(path: &Path) -> (ConversionReview, ConversionReview) {
	let request: ConversionReview =
	serde_json::from_reader(File::open(path).expect("failed to open test file"))
	.expect("failed to parse ConversionReview from test file");
	let response = Person::try_convert(request.clone());
	(request, response)
	}
	fn convert_via_file(path: &Path) -> (ConversionReview, ConversionReview) {
	let request: ConversionReview =
	serde_json::from_reader(File::open(path).expect("failed to open test file"))
	.expect("failed to parse ConversionReview from test file");
	let response = Person::try_convert(request.clone());
	(request, response)
	}

Ideally, this function would be generic over the kind we want to convert. This is however currently not possible but worth a thought in the future.

[sbernauer]

I tried putting it in crates/stackable-versioned/src/test_utils.rs, the problem is that we don't know Person in there...
And currently we don't have a trait for the T::try_convert(request.clone()) function. So I'm missing how we can move the convert_via_file function

[sbernauer]

renamed in b7080ac

[Techassi]

note: Using a reference here seems a little weird. We could instead add #[derive(Copy, Clone)] to the version enum, to be able to cheaply copy the enum, which is also suggested by the docs.

Would be interesting to see if there are any performance differences. This StackOverflow post seems to suggest that copying is smaller in memory but there will be likely no performance difference.

[sbernauer]

3f9e21f