-
Notifications
You must be signed in to change notification settings - Fork 350
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(#4361): Avoid errors when components not available in Camel catalog #4402
Conversation
christophd
commented
May 22, 2023
- Allow integrations that uses components that are not listed in the Camel catalog
- Components may not be listed in the catalog for many reasons (e.g. custom components)
- Just log a warning message in case the component is missing in the catalog
Fixes #4361 |
f5b5ac1
to
8f28324
Compare
@squakez I have tried to fix the failing E2E tests could you please rerun the workflows? |
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.
The PR change is good for me. We only need to change the warning output and let it to stderr, as it was done on purpose back in time to let dry run execution to get the output and be able to redirect it to any stream. I think we should add some line of doc in https://camel.apache.org/camel-k/next/configuration/dependencies.html to explain that and how the camel dependencies from catalog are managed.
8f28324
to
f07365e
Compare
@squakez E2E test did fail again 😵💫. this is the 2nd try to fix it. I also added some words to the dependencies page in the documentation |
Yes, [1] it's failing with:
IIUC the test want to run an integration, even when a |
… catalog - Allow integrations that uses components that are not listed in the Camel catalog - Components may not be listed in the catalog for many reasons (e.g. custom components) - Just log a warning message in case the component is missing in the catalog - Add some documentation on Camel dependency resolution
f07365e
to
123154a
Compare
…able - Make sure to use camel-quarkus extension for all Camel components that actually do have an extension available (in the catalog) otherwise fallback to arbitrary Camel dependency - Avoids to resolve Camel dependencies to camel-quarkus-xxx dependency when there is no such Camel Quarkus extension available
@squakez I have found another issue and fixed it in this PR. The issue is related to Camel dependencies that are not available in Camel Quarkus extensions. These dependencies should not get resolved to I noticed this in the newly created E2E tests where |
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.
One thing to note is that throwing some meaningful error or warning when specifying a non-existent or wrong component dependecy is also a request from another user (such as #3127). Please make sure this change also satisfies the request.
Also, as I remember it, the last discussion was that all valid dependencies for camel-k should be listed in the catalog. So if I'm not mistaken it seems to be rather a problem in the Camel catalog. I'm wondering why we can't fix the catalog in the first place.
What does it mean to use a |
thanks @tadayosi for your comments.
+1 but recent history has shown that the catalog may be incomplete (see apache/camel-k-runtime#1029 which fixed that)
I think the raw Camel component is not optimized for Quarkus but still is able to run isn't it? Of course using the Camel Quarkus extensions should be the first choice and the catalog makes sure to use those extensions once they are listed. But we should not fail when the user explicitly uses a non-listed Camel dependency. This would then prevent any customized Camel components and other things that are not listed. In the past I have also been requesting the error based on the catalog checks but today I think this is too restrictive. |
I think that the usage of non "quarkified" dependencies/components should not be supported but neither restricted if the user, for any reason, want to use such dependency. I think the approach we are taking here is to tentatively run the Integration, reporting the warning. At this stage I am wondering if it makes sense to include an Integration condition to specify that. I think would be the neatest approach. Basically, if an integration fails, the user can quickly discover the reason of the failure (ie, non supported component) in the Integration conditions. |
Was it already committed? I can see the same error still in the checks. |
I think we should made a distinction between:
About case 1, we should allow the user to add such dependency as it could be a component/extension non part of the camel distribution but still a valid component/extensions. However in such case I think the user should define the component using a maven coordinate and the component should not use the ´camel.apache.org` as artifact group which should bypass the catalog check (at least this is what I recall). Does this make any sense ? |
@squakez yes, still same error in E2E test. I am diving deeper into this locally in order to provide a fix |
@lburgazzoli sounds good. How do we identify those banned dependencies in the first place? |
@squakez @lburgazzoli @tadayosi sorry, resolving to So that makes me think that we should maybe go back to throwing errors when the Camel dependency is not in the catalog. In apache/camel-k-runtime#1029 we have fixed the issue that the catalog does not list miscellaneous Camel dependencies and this is the root cause for the initial issue reported in #4361 I think @lburgazzoli made a good point. When a user needs to use a custom Camel component it should not be using the Camel So maybe I close this PR and we keep things as they have been with throwing errors once the Camel dependency is not resolvable via the catalog. Sorry folks for the back and forth. WDYT? |
I think we need to ask the quarkus team, @jamesnetherton @ppalaga any hint ? |
I see several options:
|
In this case the |
@claudio4j yes, but what should be the proper version value? At the moment the operator will overwrite all user defined versions on Camel dependencies in order to force the versions coming from the bom. We would need to also manage the raw Camel version in the catalog to actually set the proper version |
I think we can close this one in favor of using #4411 which keeps the logic to throw errors when a Camel dependency is not part of the catalog |
For the record, the Camel version is present in Camel Quarkus catalog under
Of course, there is no catalog entry for the missing components, but the |