Split JSON patch out into a new module

[gravesm]

SUMMARY

JSON patch in the k8s module has likely never worked. Furthermore,
having resource_definition double as the parameter for patch operations
makes fixing this in k8s difficult. This is due, in part, to the fact
that a single patch consists of a list of operations that should be
processed as a whole, while resource_definition supports a list of
resources that should be processed separately. The merge_type parameter
allows more than one merge strategy to be specified, but there is no
case where a JSON patch could work for JSON merge or strategic merge,
nor the other way around.

This commit moves JSON patch functionality out into a separate module.
Check mode support is provided through the third party jsonpatch
library.

ISSUE TYPE

• New Module Pull Request

COMPONENT NAME

k8s_json_patch

ADDITIONAL INFORMATION

[codecov]

Codecov Report

Merging #99 (4ef21ac) into main (0bbc9ca) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #99   +/-   ##
=======================================
  Coverage   24.02%   24.02%           
=======================================
  Files           1        1           
  Lines         154      154           
  Branches       29       29           
=======================================
  Hits           37       37           
  Misses        112      112           
  Partials        5        5           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0bbc9ca...4ef21ac. Read the comment docs.

[jillr]

Suggested change

	minor_changes:
	- k8s_json_patch - split JSON patch functionality out into a separate module (https://github.com/ansible-collections/kubernetes.core/pull/99).
	major_changes:
	- k8s - the JSON patch functionality has been removed. This feature never worked.
	- k8s_json_patch - split JSON patch functionality out into a separate module (https://github.com/ansible-collections/kubernetes.core/pull/99).

If this truly never worked and this won't change any interfaces/behaviour for existing playbooks that might be trying to use this it doesn't really need to be deprecated, but let's still leave a clear paper trail for users.

[gravesm]

I have gone ahead and just deprecated merge_type=json. While it never worked, it usually failed silently.

[Akasurde]

Rest LGTM. Feel free to merge after resolving the comments.

[abikouo]

I am thinking about having a k8s_patch module instead dealing with all related patch stuffs like the kubectl patch command. However for the definition used for strategic merge can also be used to create an object for now, but we should remove this feature to have a module k8s_patch patching only existing objects and k8s used to create them.

[gravesm]

@abikouo I'm unclear what you are suggesting. Are you saying you disagree with this module as a whole and you don't think we should add it? As I commented here, I think removing all the patch functionality from the k8s module is going to be pretty controversial.

[fabianvf]

nice! lgtm, and I think it makes sense to have json-patch be its own module with state=patched in the main module. json-patch is just so different it would be kind of awkward even in a shared patch module

[fabianvf]

re: #99 (comment)

One thing to note is that the other modules do return result [0][1]. If result isn't appropriate we should probably change the return value in the other modules as well for 3.0, and it would be nice if it matched what was output here (so probably not patched_output). As a user I think it would be nicer if there was a consistent return value key I could check, rather than having to check/remember what the output key for the same type of information is for each module separately. Perhaps k8s_object or something like that would make sense.

0: https://github.com/ansible-collections/kubernetes.core/blob/main/docs/kubernetes.core.k8s_module.rst#return-values
1: https://github.com/ansible-collections/kubernetes.core/blob/main/docs/kubernetes.core.k8s_scale_module.rst#return-values

[fabianvf]

Could we add a case that tests the remove action and verifies it's idempotent?

[gravesm]

Hmm, I'm not sure if we can test idempotency for remove ops. The path won't exist the second time around and kubernetes will return an error.

[gravesm]

@fabianvf false alarm. result is not a reserved keyword. Reverting back to result.

[Akasurde]

LGTM