Add implicit model generation for load_all_yaml and from_dict
#34
Conversation
When trying to create an unknown resource (a resource not in the core kubernetes set or previously defined as a GenericResource) using codecs.from_dict or codecs.load_all_yaml, this change attempts to generate a generic model for the unknown resource. Using a provided, initialized lightkube Client, 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 behaviour is unchanged.
…ployment) Previously, the code would try to create a generic model for native objects like apps/v1/deployment. This change delays attempting to infer a model until after we've tried to import the model as if it is a fully modeled by lightkube (eg: not generic)
|
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) |
|
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 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 |
|
Right, for that we can have a parameter as you mention that just create any CRD encountered during load of the YAML stream. # 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. |
…ps/v1/deployment)" This reverts commit e5841cc.
…ad_all_yaml" This reverts commit 2e7e562.
generic_resource.load_in_cluster_generic_resources automates creating generic resources for all custom resources in a cluster. This is done by listing all CRDs available from a provided Client, creating a new generic resource for each version of each resource.
Adds an optional create_resources_for_crds boolean parameter to codecs.load_all_yaml. If False (default), there is no change in behaviour of load_all_yaml. If create_resources_for_crds==True, the load_all_yaml will create generic resources for any CRDs it loads.
Merges in code from commit 0328fdf
|
I've tried to refactor based on your suggestions. What do you think? |
|
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! |
* Improve docstrings * Remove some dependencies on apiextensions_v1 * expose new public generic_resource functions * add async_load_in_cluster_generic_resources and test * remove direct load of lightkube.models
|
Docs not forgotten but ran out of time today. Will update in the next commit |
lightkube/core/internal_models.py
Outdated
| @@ -1,7 +1,7 @@ | |||
| import sys | |||
|
|
|||
| try: | |||
| from ..models import meta_v1, autoscaling_v1 | |||
| from ..models import meta_v1, autoscaling_v1, apiextensions_v1 | |||
There was a problem hiding this comment.
In case of import failure we need to define a mock for each method/class used in apiextensions_v1. See bottom of this same file for examples.
The asyncmock package (needed for python <3.8) works differently from the builtin in 3.8. Commented out part of the test to handle that.
|
Good progress!
I was also thinking that we may need an integration test for |
lightkube/core/internal_models.py
Outdated
| try: | ||
| from ..models import apiextensions_v1 as apiextensions | ||
| except: | ||
| from ..models import apiextensions_v1beta1 as apiextensions |
There was a problem hiding this comment.
In case of import failure we need to define a mock for each method/class used in apiextensions_v1. See bottom of this same file for examples (copied over comment as it was not visible any longer).
There was a problem hiding this comment.
Just FYI, you may get a conflict on this file as #37 was merged.
There was a problem hiding this comment.
Ok I misunderstood this. I think now I've got it
# Conflicts: # lightkube/core/internal_models.py
Addresses review feedback about missing mocks.
Also fixes bugs found by the e2e tests
Correctly handles mocking for the async for loops and non-async AsyncClient.list() method
Previously, test_load_in_cluster_generic_resources used a CRD for CronTab that was shared with another test. If that CRD was applied and not deleted in a previous test, test_load_in_cluster_generic_resources fails because it asserts the CRD does not exist at the start of the test. This change makes a new (very similar) CRD so we cannot have this collision.
Previously, cleanup of resources in test_load_in_cluster_generic_resources_* did not wait for their resources to be cleaned up before completion, sometimes resulting in test_load_in_cluster_generic_resources_async starting too fast and failing because it sees a leftover resource from the previous test. This adds a small persistence to the tests to avoid short, transient deletion delays
…s_async Avoids resource collision by randomly naming the CRDs for testing.
…s_async Avoids resource collision by randomly naming the CRDs for testing.
|
Added e2e tests for both regular and async clients and glad you asked because both had bugs :D I was using Ran into a few problems in the tests where I hit race conditions between the |
|
Docs are added now as well. Thanks for all the great feedback! |
|
Changes merged, great work!
|
|
ty for the helpful reviews! and makes sense for |
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
clientargument tocodecs.from_dictorcodecs.load_all_yaml. Iffrom_dictorload_all_yamlare passed aClientvia 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 aGenericResourcefor it if found. If aClientis not provided tocodecs.from_dictorcodecs.load_all_yaml, the existing behaviour is unchanged.