-
Notifications
You must be signed in to change notification settings - Fork 235
feat: support additional java CRDs in test extension #3163
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
feat: support additional java CRDs in test extension #3163
Conversation
Adds withAdditionalCustomResourceDefinition(CustomResourceDefinition definition) to LocallyRunOperatorExtension#Builder. These definitions are applied at the same time as additionalCustomResourceDefinitions. The definitions are deleted according to the existing lifecycle. Why: In some cases we may wish to apply a CustomResourceDefinition that has no corresponding CRD file. For example, some frameworks publish fabric8-based api modules which contain only the java classes and no CRD files. Signed-off-by: Robert Young <robertyoungnz@gmail.com>
| public void applyCrd(CustomResourceDefinition customResourceDefinition) { | ||
| try { | ||
| String resourceTypeName = customResourceDefinition.getMetadata().getName(); | ||
| final var pathAsString = crdMappings.get(resourceTypeName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure whether this logic should be applied.
For the other mechanism withAdditionalCustomResourceDefinition(Class<? extends CustomResource> customResource) { it checks the custom crdMappings first and prefers to apply a CRD supplied there, else it looks in the standard location.
What should we do if the crdMappings contains a CRD with the same name as this CustomResourceDefinition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, we should check if we already applied CRD with same name, if yes throw an exception, since probably that is not what user intends. This can be a separate PR.
|
@xstefank pls take a look also |
csviri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| public void applyCrd(CustomResourceDefinition customResourceDefinition) { | ||
| try { | ||
| String resourceTypeName = customResourceDefinition.getMetadata().getName(); | ||
| final var pathAsString = crdMappings.get(resourceTypeName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, we should check if we already applied CRD with same name, if yes throw an exception, since probably that is not what user intends. This can be a separate PR.
|
@robobario @xstefank if you have the bandwidth and could do the duplicate detection would be great, but mainly thank you for this PR. |
|
@robobario if that helps we can have a patch release this week for you |
Adds
withAdditionalCustomResourceDefinition(CustomResourceDefinition definition)toLocallyRunOperatorExtension#Builder. These definitions are applied at the same time as the existingadditionalCustomResourceDefinitions. The definitions are deleted according to the existing lifecycle.Resolves #3004
Why:
In some cases we may wish to apply a CustomResourceDefinition that has no corresponding CRD file. For example, some frameworks publish fabric8-based api modules which contain only the java classes for their CRDs, and no CRD files.