fiaas / k8s

Python client library for the Kubernetes API
https://fiaas.github.io/
Apache License 2.0
39 stars 24 forks source link

Move status to a class #118

Closed herodes1991 closed 12 months ago

herodes1991 commented 1 year ago

When trying to add an empty dict to the status, the JSON formatter we use deletes the field. As this is needed to enable the /status endpoint, we are changing the definition to create an empty class instead of a dict.

herodes1991 commented 1 year ago

Here you can see the problem: https://github.com/fiaas/fiaas-deploy-daemon/tree/add_status_endpoint_followup The tests are falling because we expect a

subresources: {
   status: {}
}

but it's receiving a

subresources: {}

expected result: https://github.com/fiaas/fiaas-deploy-daemon/blob/add_status_endpoint_followup/tests/fiaas_deploy_daemon/crd/test_crd_resources_syncer_apiextensionsv1.py#L49 Model created: https://github.com/fiaas/fiaas-deploy-daemon/blob/add_status_endpoint_followup/fiaas_deploy_daemon/crd/crd_resources_syncer_apiextensionsv1.py#L47 (tried also with dict())

oyvindio commented 12 months ago

I took a look at this PR and did some testing locally this morning; I see that there were some follow up PRs (#119 , #120) as well.

The behavior needed to handle the status subresource format expected by the API, as mentioned here https://github.com/fiaas/k8s/pull/118#issuecomment-1772560289, doesn't seem entirely straightforward to express in fiaas/k8s Models, so I thought I would just share my observations in case it is helpful; The issue I see is that Model.as_dict() returns None when none of the fields on the model are set (see this condition https://github.com/fiaas/k8s/blob/master/k8s/base.py#L259-L260). On a Model which doesn't have any fields, such as CustomResourceSubresourceStatus in this PR, as_dict always returns None; this is a little difficult to work with when the API expects an empty object.

As I see it, both the additional CRD subresources should probably be disabled by default (since this is the current behavior), and it has to be possible to enable and disable (if I understand the changes in #120 correctly, it seems to always enable the status subresource(?)) the resource by changing how the parameters to CustomResourceSubresources are set. First of all, I think it would be useful to add some tests where a CustomResourceDefinition model is created with the different configurations mentioned above, and the output of its as_dict method (i.e. what will be sent to the API) is verified against the expected result.

Maybe one way to make this work might be to have two different types (via Field.alt_type) for the CustomResourceSubresources.status field; for example CustomResourceSubresourceStatusEnabled and CustomResourceSubresourceStatusDisabled. CustomResourceSubresourceStatusEnabled can have its as_dict method overridden to always return an empty dict, and then I think CustomResourceSubresourceStatusDisabled can look like CustomResourceSubresourceStatus does here ("empty" Model class). I think a configuration like this should make it possible to both enable and disable the status resource, depending on whether the -Enabled or -Disabled type is used.