-
Notifications
You must be signed in to change notification settings - Fork 29
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 Kamelet Beans #757
Conversation
); | ||
|
||
return isSupported ? ( | ||
<> | ||
<MetadataEditor name="Beans" schema={beansSchema} metadata={getBeansModel()} onChangeModel={onChangeModel} /> |
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.
There might be a reason I made it always getting the model from the entity at first, I'll get this back.
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.
Done
Fixes: KaotoIO#540 Fixes: KaotoIO#642
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.
Looks good 👌 , thanks @igarashitm
packages/ui/src/models/camel/__snapshots__/kamelet-resource.test.ts.snap
Show resolved
Hide resolved
let model: any; | ||
let beansHandler: BeansEntityHandler; | ||
beforeEach(() => { | ||
model = { ...routeStub.camelRouteJson }; |
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 think it's better to clone the camelRouteJson
object, just in case we're performing modifications on it. We could use cloneDeep
from lodash or structuredClone
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.
ugh right, Done 👍
setBeansModel(model: BeansDeserializer | RouteTemplateBeanDefinition[]) { | ||
switch (this.type) { | ||
case 'beans': { | ||
if (model?.length > 0) { | ||
let entity = this.getBeansEntity() as BeansEntity; | ||
if (!entity) { | ||
entity = (this.beansAware as BeansAwareResource).createBeansEntity(); | ||
} | ||
entity.parent.beans = model; | ||
} else { | ||
const entity = this.getBeansEntity(); | ||
entity && this.deleteBeansEntity(entity); | ||
} | ||
break; | ||
} | ||
case 'routeTemplateBean': | ||
if (model?.length > 0) { | ||
let entity = this.getBeansEntity() as RouteTemplateBeansEntity; | ||
if (!entity) { | ||
entity = this.createBeansEntity() as RouteTemplateBeansEntity; | ||
} | ||
entity.parent.beans = model; | ||
} else { | ||
const entity = this.getBeansEntity() as RouteTemplateBeansEntity; | ||
entity && this.deleteBeansEntity(entity); | ||
} | ||
break; | ||
} | ||
} |
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.
Is it correct to assume that the main difference between both types is how the entity
object is assigned?
For beans
:
entity = (this.beansAware as BeansAwareResource).createBeansEntity();
For routeTemplateBean
:
entity = this.createBeansEntity() as RouteTemplateBeansEntity;
Could something like this work?
setBeansModel(model: BeansDeserializer | RouteTemplateBeanDefinition[]): void {
if (!Array.isArray(model) || model.length === 0) {
const entity = this.getBeansEntity();
entity && this.deleteBeansEntity(entity);
return;
}
let entity = this.getBeansEntity();
if (!entity) {
if (this.type === 'beans') {
entity = (this.beansAware as BeansAwareResource).createBeansEntity();
} else {
entity = this.createBeansEntity() as RouteTemplateBeansEntity;
}
}
entity.parent.beans = model;
}
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.
Yes these were redundant, this.getBeansEntity()
and this.createBeansEntity()
already do the switch-case and this could rely on that - fixed 👍
@@ -0,0 +1,187 @@ | |||
import { BeansAwareResource, CamelResource, RouteTemplateBeansAwareResource } from '../../camel'; |
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.
Could we add a few tests for this class if possible?
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.
Added tests for RouteTemplateBeansAwareResource
methods into kamelet-resource.test.ts
👍 For BeansAwareResource
we already have some tests in camel-route-resource.test.ts
.
- Use lodash cloneDeep to duplicate the test stubs - Clean up BeansEntityHandler.setBeansModel() - Add tests for RouteTemplateBeansEntity handling in KameletResource
Fixes: #540
Fixes: #642