Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(kuma-cp) create new object from resource descriptor #3114

Merged
merged 1 commit into from
Nov 10, 2021

Conversation

jakubdyszkiewicz
Copy link
Contributor

Summary

It is not an issue with the current code but it's a trap that we can easily fall into.

ResourceDescriptor has a method NewObject(), but in order to call it, it needs to set internal objectType and listType fields. Those fields are set by InitDescriptor() method which is only called when the descriptor is put into ResourceRegistry.

The problem is that the ResourceDescriptor can also be retrieved from .Descriptor() method of the object and this copy of an object does not contain initiated objectType and listType which results in panic.

I moved the retrieving resource type to NewObject itself, I don't think we should be able to create an object which requires extra initialization, especially if the object is copied around instead of passing a pointer.

The alternative would be to template resource-gen with model.InitDescriptor when the descriptor is generated in the code, but I don't like the fact we can create a struct that is not functional.

Issues resolved

No issues.

Documentation

  • No docs, internal changes.

Testing

  • Unit tests
  • E2E tests
  • Manual testing on Universal
  • Manual testing on Kubernetes

Backwards compatibility

  • No backporting

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@jakubdyszkiewicz jakubdyszkiewicz requested a review from a team as a code owner November 9, 2021 17:31
Copy link
Contributor

@lahabana lahabana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a sensible change!

Copy link
Contributor

@jpeach jpeach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good find!

@jakubdyszkiewicz jakubdyszkiewicz merged commit b4a13ae into master Nov 10, 2021
@jakubdyszkiewicz jakubdyszkiewicz deleted the fix/obj-descriptor-new-obj branch November 10, 2021 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants