fix(#3896): Fix dependency inspector supporting property placeholders

[christophd]

Do not raise errors during dependency inspection when using property placeholders in toURI. Fixes http-sink Kamelet

Fixes #3896

[christophd]

FYI @tadayosi

[tadayosi]

Thanks a lot Christoph for taking care of this issue!

As already commented in the review, I don't think checking only : in uri can cover all edge cases. For example:
https://github.com/apache/camel-kamelets/blob/main/kamelets/elasticsearch-search-source.kamelet.yaml#L110

            uri: "{{local-es}}:{{clusterName}}"

This kamelet has uri that contains : but still uses a property placeholder. A better logic should be:

1. Try to decode the uri.
2. Check the returned scheme; if it's nil, check the uri on whether a placeholder is used at the place of scheme.
3. If a placeholder is used return silently; otherwise throw an error.

[tadayosi]

Thinking a bit further, what should we really do with the use of Kamelet placeholders in terms of the dependency resolution? The original intention why we've added a stricter component dependency validation is to improve UX in case of wrong components used in the user's integration, so that Camel K can give users early feedback on possible misconfigurations in user side.

Since it is related only to Kamelets and Kamelets already have an explicitly defined set of dependencies, so dependency resolution is not a problem in this case, right? Is it just fine to skip it silently and everything should work fine?

[christophd]

I wonder if throwing an error at this state when the uri does not resolve to a component in the Catalog is the right thing to do in general. Using placeholders in the uri may be only one scenario where throwing an error because of this is inappropriate.

What about uri that for some other reason does not resolve to a component that we know. We also support jitpack dependencies and custom Maven repositories that might ship libraries with custom components that are not known to the Catalog. All these will raise the error here or am I missing something?

[tadayosi]

What about uri that for some other reason does not resolve to a component that we know. We also support jitpack dependencies and custom Maven repositories that might ship libraries with custom components that are not known to the Catalog. All these will raise the error here or am I missing something?

I think it's a good question, and I raised the same at #3449 (comment). In Camel, users can even change the scheme name for the existing components.
https://camel.apache.org/manual/component.html#_programmatically

At that time, I took that Camel K doesn't allow such use cases like extending with their own components or simply renaming the components. We assume every valid component should be present in the Camel Quarkus and Catalog. But we can rethink about the assumption and explore what level of flexibility Camel K can provide to users regarding extensibility of components.

Can you think of any other cases where throwing an error at this state might cause issues?

[tadayosi]

I wonder if throwing an error at this state when the uri does not resolve to a component in the Catalog is the right thing to do in general. Using placeholders in the uri may be only one scenario where throwing an error because of this is inappropriate.

And yes, what I've implemented for 1.11.0 might be a bit too stricter and less flexible when handling dependencies. Possibly we should move the dependency check at a later stage. We can revisit it as well.

[christophd]

back from PTO 😄 so I enhanced the PR to also cover URIs like {{scheme}}:{{resource}}. The inspector will now ignore URIs that are not resolvable due to a scheme using a placeholder. Added some unit tests to cover some scenarios.

@tadayosi @squakez please have another look

[tadayosi]

Looks great. Thanks @christophd !

[christophd]

The CI job failing because of TestLocalBuildWithInvalidDependency but the error message says this is due to a test that has been running before:

panic: Fail in goroutine after TestLocalBuildWithTrait has completed

Does that sound familiar to somebody?

Could someone please rerun the failing CI job so we make sure this has been a temp hiccup

[squakez]

Re-running it.

[christophd]

@squakez thank you!

YAY! all green!