swiftkube / client

Swift client for Kubernetes
Apache License 2.0
129 stars 20 forks source link

adds suspend and unsuspend functions to Cronjob and implements PATCH #15

Closed thomashorrobin closed 1 year ago

thomashorrobin commented 2 years ago

This PR adds functions to make it easier and more reliable to update corncobs with Swiftkube.

Using PATCH is important as it enables us to update resources without concern for copying and uploading the entire resource. Which I have found to be error prone and messy. Implementing PATCH makes this much easier

@iabudiab as always, this may need some further work as I'm not as familiar with Swift as I'd like to be. And I'm not sure my implementation of PATCH is up to the standard it would need to be to be able to merged. So happy to rewrite if you have suggestions

iabudiab commented 2 years ago

@thomashorrobin Thanks a lot fir the PRs!!! and sorry for the delayed response on my part. I was on a much-needed and long-overdue vacation, where I stayed away from the digital world 😬

I'll take a look in the following days and let you know, what my thoughts are. I've been slowly working on a patch implementation myself, and like you said, it is not an easy task, especially since k8s has 3 kinds of patches, that are supported.

thomashorrobin commented 1 year ago

Yeah, the 3 kinds of patches thing is difficult. I would love to have a proper chat about it sometime!

iabudiab commented 1 year ago

Yeah, the 3 kinds of patches thing is difficult. I would love to have a proper chat about it sometime!

Gladly! I'll try to summarise my attempts and thoughts about this sometime during the week, and maybe we can ping-pong some ideas.

thomashorrobin commented 1 year ago

@iabudiab I think this PR is dead, I'm probably going to close it. But I think we do need to do something about PATCH. From my point of view I want to be able to easily remove and add labels, rename CronJobs, edit port mappings, etc. As you point out, the 3 kinds of patches thing complicates any solution here.

Have you had any thoughts about what an abstraction for patch would look like here?

iabudiab commented 1 year ago

@thomashorrobin ok, no problem. And yes, we have to do something about PATCH. I wanted to summarise my efforts up until now, but just couldn't get to it 😅

I think, the easiest option to provide would be to allow arbitrary JSON as a payload and rely on the strategic merge patch strategy. This would cover lots of uses cases without being complicated. However, we won't have any type-safety with this approach. After that generic PATCH API, I'd like to provide some kind of type-safety somehow (still working on that)

JSON Merge, i.e. RFC 7368, would be the next step, and JSON Patch, i.e. RFC 6902 the last.

t089 commented 1 year ago

One way which I used to implement PATCH in the past, is to start from some model, eg an empty Pod() then generate JSON for that object. Next you copy the object and modify it and generate JSON again. Now you can compare the JSON and generate a JSON merge patch.

So, a type-safe PATCH api would always need to take an "old" and a "new" value and would then be able to defer the PATCH from differences and send it to the server.

thomashorrobin commented 1 year ago

I like the idea of modifying an object, and then getting the merge patch from the diff of the new and old objects. It gives us the fellability and type-safety we want.

thomashorrobin commented 1 year ago

Anyway, I need to close this PR as we're going in a different direction

iabudiab commented 1 year ago

@thomashorrobin @t089

Exactly. Here is what I've accomplished so far. It still has some quirks and edge-cases, that do not work: