Add implicit model generation for `load_all_yaml` and `from_dict`

ca-scribner commented 2 years ago

Foreward

This PR proposes some convenience for interacting with batches of yaml. I'm not married to the API, etc., and the code does not have a full test suite, but just curious if you're interested in a feature like this and if so what suggestions/requests you have on the API. If you're interested, I'll flesh it out further.

Proposed feature

Sometimes, code might want to apply a YAML file that has CustomResources it might not know about (or might not otherwise need to know about). Currently, we need to instantiate generic resources for these objects before passing their definitions to the cluster for action (say to create an instance of a CustomResource). This proposal tries to make using CustomResources from YAML a bit more convenient by adding an optional client argument to codecs.from_dict or codecs.load_all_yaml. If from_dict or load_all_yaml are passed a Client via this arg, whenever they find an unknown generic resource they try to implicitly load that resource's definition from the cluster. If this does not work, they fail like before. This allows for the application of YAML without needing to know ahead of time what CRs might be defined in the yaml.

Implementation details

We client.list(CustomResourceDefinition) to look for the resource we are currently missing, creating a GenericResource for it if found. If a Client is not provided to codecs.from_dict or codecs.load_all_yaml, the existing behaviour is unchanged.

gtsystem commented 2 years ago

Interesting idea. I was thinking we can make this more reusable and explicit just adding a function to load CDRs from the cluster.

Usage example:

from lightkube import Client
from lightkube import generic_resource as gr

client = Client()
gr.load_in_cluster_generic_resources(client)   # here we explicitly load all CRDs, we can also return them 

...
with open('deploy.yaml') as f:
    codecs.load_all_yaml(f)

Same can be used to get a resource to be used for read/write:

from lightkube import Client
from lightkube import generic_resource as gr

client = Client()
gr.load_in_cluster_generic_resources(client)
CronJob = gr.get_generic_resource("stable.example.com/v1", "Job")  # get the class representing this resource.
client.get(CronJob)

ca-scribner commented 2 years ago

Sorry for the slow response.

I like the idea of a function specific for this purpose of loading the cluster's resources, that sounds like a nice helper. And it integrates well with get_generic_resource as you show.

What about the case where YAML both defines a CRD and then creates a CR for that definition? In that situation, pre-loading the generics will not be enough. The only thing I can think of for that situation is catching the error in the load_all_yaml and trying to get the CRD from the cluster at that time

gtsystem commented 2 years ago

Right, for that we can have a parameter as you mention that just create any CRD encountered during load of the YAML stream. Something like:

# load_in_cluster_generic_resources should be called first for resources not defined in the file ...

with open('deploy.yaml') as f:
    codecs.load_all_yaml(f, create_missing_resources=True). # whenever a CRD is encountered in the file, it's created if missing.

ca-scribner commented 2 years ago

I've tried to refactor based on your suggestions. What do you think?

ca-scribner commented 2 years ago

All sounds reasonable! I've focused my spare time on the other PR the past few days and will be out of town this week, but will address these suggestions next week. Thanks!

ca-scribner commented 2 years ago

Docs not forgotten but ran out of time today. Will update in the next commit

gtsystem commented 2 years ago

Good progress! We have few things left:

One comment regarding the mocks for mkdocs
The documentation for the new public functions exposed in codecs and in generic_resources.

I was also thinking that we may need an integration test for load_in_cluster_generic_resources() as it uses the client. So create a CRD, wait it to be available and call load_in_cluster_generic_resources().

ca-scribner commented 2 years ago

Added e2e tests for both regular and async clients and glad you asked because both had bugs :D I was using models when I should have been using resources for the CRDs, but I think that is good now. I also noticed that the AsyncClient.list() method is not async - is that on purpose or a bug? If a bug and it should be async, async_load_in_cluster_generic_resources will need a minor tweak

Ran into a few problems in the tests where I hit race conditions between the client.delete(crd) calls of one test and the start of another test, but I think I have those resolved now by just making randomly named CRDs for the tests.

ca-scribner commented 2 years ago

Docs are added now as well. Thanks for all the great feedback!

gtsystem commented 2 years ago

Changes merged, great work!

AsyncClient.list() returns an async generator which will start executing once iterating. So iteration requires await but the call to list itself doesn't.

ca-scribner commented 2 years ago

ty for the helpful reviews! and makes sense for AsyncClient.list(), thanks!

gtsystem / lightkube