Add test for CRD with status subresource and without

[herodes1991]

Added test and improvements commented in PR#118.

[oyvindio]

It is really nice to have the added tests for the various subresource configuration, thanks for that! I think the current state of this changeset is a step in the right direction; I have left some suggestions in the inline comments that I think could improve the usability / readability of the API, please take a look

[oyvindio]

These two functions are almost the same, so they could be simplified; I think if e.g. _create_subresource_with_status_enabled_crd is modified to take subresources as its parameter (and renamed to indicate the change in behavior), then create_subresource_with_status_disabled_crd wouldn't be necessary.

[oyvindio]

The behavior in this case is a bit confusing to me because of how the models are named and composed; CustomResourceVersion(subresources= CustomResourceSubresourcesStatusEnabled(), ...) reads as if the status subresource would be enabled, but both scale and status subresources are disabled. See suggestion in comment above

[oyvindio]

I think is an improvement on the current behavior (in master), since it makes it possible to enable/disable the scale and status resources independently. However, there are some cases where the behavior with the Model components in this PR seems confusing to me;

• If subresources is set to CustomResourceSubresourcesStatusEnabled(), it reads to me as if the status subresource would be enabled, but both scale and status are disabled in this configuration.
• If subresources was set to CustomResourceSubresourcesStatusEnabled() as above, and the scale parameter is added, so it is set to something like CustomResourceSubresourcesStatusEnabled(scale=CustomResourceSubresourceScale(...)), both scale and status subresources are now enabled. In isolation this makes sense, but it is a bit odd that adding the scale parameter also enables the previously disabled status resource.

I think one way to make the behavior expressed by the API clearer is by pushing the Enabled/Disabled types "down" one level, to the type of the CustomResourceSubresources.status field. Something like this:

class CustomResourceSubresourceStatusEnabled(Model):
    def as_dict(self):
        return {}

class CustomResourceSubresourceStatusDisabled(Model):
    pass

class CustomResourceSubresources(Model):
    scale = Field(CustomResourceSubresourceScale)
    status = Field(CustomResourceSubresourceStatusDisabled, alt_type=CustomResourceSubresourceStatusEnabled)

With the above components, the different configuration options for subresources would look like this

# 1) (default) scale disabled, status disabled
None

# 2) scale disabled, status disabled (both implicitly)
CustomResourceSubresources()

# 3) scale enabled, status disabled
CustomResourceSubresources(scale=CustomResourceSubresourceScale(...))

# 4) scale disabled, status enabled
CustomResourceSubresources(status=CustomResourceSubresourceStatusEnabled())

# 5) scale enabled, status enabled
CustomResourceSubresources(
  scale=CustomResourceSubresourceScale(...),
  status=CustomResourceSubresourceStatusEnabled()
)

# 6) scale enabled, status disabled (explicitly)
CustomResourceSubresources(
  scale=CustomResourceSubresourceScale(...),
  status=CustomResourceSubresourceStatusDisabled()
)

As I see it, this has some benefits;

• The resulting subresource configuration is clearer just from reading how the Model objects are composed where used.
• If fields are added to the status subresource API object in the Kubernetes API later, it is slightly easier to add them here as well, since the status field takes a Model type.
• EmptyField is no longer needed.

What do you think?

[herodes1991]

I tested this, and it was not working:

class CustomResourceSubresourceStatusEnabled(Model):
    def as_dict(self):
        return {}

The Field is deleting the value because it's an empty dict 😔

For the rest, it has total sense.

[herodes1991]

That's the reason I had to create the EmptyField 😄

[herodes1991]

Will this works for you?

class CustomResourceSubresourcesStatusDisabled(Model):
    scale = Field(CustomResourceSubresourceScale)


class CustomResourceSubresourcesStatusEnabled(Model):
    scale = Field(CustomResourceSubresourceScale)
    status = {}

With this, we do not need EmptyField, and it's converted to the expected thing.

[herodes1991]

No, it's not working, sorry 😔

[herodes1991]

Check it now, improved the classes and the tests

[oyvindio]

The Field is deleting the value because it's an empty dict 😔

As I understand it, as long as the type contained by the Field has an as_dict() method, the Field's dump / _as_dict will just return what that method returns. If the type doesn't have as_dict(), there are some special cases, for example for dict. Those are probably why it is difficult to get Field(dict) to return a empty dict. I think overriding as_dict() on CustomResourceSubresourceStatusEnabled, as suggested, should make it possible to send an empty object to the API unless there is something else which transforms it to a json null.

[oyvindio]

LGTM 👍