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

Detect wire tap EIP endpoints #3312

Merged
merged 8 commits into from
Jun 2, 2022
Merged

Conversation

ammachado
Copy link

@ammachado ammachado commented May 27, 2022

Detect dependencies used on wire tap EIP endpoints

Release Note

feat: detect wire tap EIP endpoints

Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

Nice work! The feature could be really helpful. However, we should have it in parity with the other supported DSL. We still miss from xml, yaml and kotlin. If you can give it a try and implement there it would be great. Otherwise we'd be in a situation where we support the feature in certain DSL only.

@ammachado
Copy link
Author

I think now it also fixes #2139.

Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

I had a better look following the yaml implementation. The problem with this implementation is that we don't really add the component dependency needed later by runtime. In order it to work, we probably need to include a check in the generic inspector.go. There you can see what we are doing with other functionalities, ie, checking for a particular expected regexp and including the dependency needed. I think this would be the proper way to go. In order to double check that, you need to include in your unit tests a validation of the expected dependency.

pkg/util/source/inspector_yaml.go Show resolved Hide resolved
pkg/util/source/inspector_yaml_test.go Show resolved Hide resolved
Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

I gave a further look after your answers. I think we can proceed with this, but I'd add some further test to confirm the components provided into the wiretaps are correctly retrieved. Right now you've just tested kamelets:xyz. If you can add another unit test to check some other component (ie, wiretap(jms:xyz)) and verify that the component dependency is indeed added into the Integration. Also, a simple usage example (under /examples dir) would be great to show the users how to use the new feature.

@squakez squakez dismissed their stale review June 1, 2022 05:51

Answers provided are good

@squakez squakez added the kind/feature New feature or request label Jun 1, 2022
@squakez squakez merged commit 44ce4b9 into apache:main Jun 2, 2022
@squakez
Copy link
Contributor

squakez commented Jun 2, 2022

Thanks for your contribution!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants