You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by GitBox <gi...@apache.org> on 2022/12/14 09:32:24 UTC
[GitHub] [camel-k] christophd opened a new pull request, #3901: fix(#3896): Fix dependency inspector supporting property placeholders
christophd opened a new pull request, #3901:
URL: https://github.com/apache/camel-k/pull/3901
Do not raise errors during dependency inspection when using property placeholders in toURI. Fixes http-sink Kamelet
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez merged pull request #3901: fix(#3896): Fix dependency inspector supporting property placeholders
Posted by GitBox <gi...@apache.org>.
squakez merged PR #3901:
URL: https://github.com/apache/camel-k/pull/3901
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on pull request #3901: fix(#3896): Fix dependency inspector supporting property placeholders
Posted by GitBox <gi...@apache.org>.
squakez commented on PR #3901:
URL: https://github.com/apache/camel-k/pull/3901#issuecomment-1376931935
Re-running it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on pull request #3901: fix(#3896): Fix dependency inspector supporting property placeholders
Posted by GitBox <gi...@apache.org>.
christophd commented on PR #3901:
URL: https://github.com/apache/camel-k/pull/3901#issuecomment-1376925614
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on pull request #3901: fix(#3896): Fix dependency inspector supporting property placeholders
Posted by GitBox <gi...@apache.org>.
christophd commented on PR #3901:
URL: https://github.com/apache/camel-k/pull/3901#issuecomment-1376982365
@squakez thank you!
YAY! all green!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] tadayosi commented on pull request #3901: fix(#3896): Fix dependency inspector supporting property placeholders
Posted by GitBox <gi...@apache.org>.
tadayosi commented on PR #3901:
URL: https://github.com/apache/camel-k/pull/3901#issuecomment-1354299231
> 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 https://github.com/apache/camel-k/issues/3449#issuecomment-1226854358. 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.
Can you think of any other cases where throwing an error at this state might cause issues?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] squakez commented on a diff in pull request #3901: fix(#3896): Fix dependency inspector supporting property placeholders
Posted by GitBox <gi...@apache.org>.
squakez commented on code in PR #3901:
URL: https://github.com/apache/camel-k/pull/3901#discussion_r1048406955
##########
pkg/util/source/inspector.go:
##########
@@ -336,6 +336,11 @@ func (i *baseInspector) discoverKamelets(meta *Metadata) {
}
func (i *baseInspector) addDependencies(uri string, meta *Metadata, consumer bool) error {
+ if !strings.Contains(uri, ":") {
Review Comment:
Not sure if this statement is okey to be here. I mean, in theory we may expect an uri with the form `mycomponent`, without the scheme. I wonder if it makes sense: 1) to check for the specific use case (`{{}}`) directly in the `DecodeComponent()` func, 2) if it makes sense to have the check in the callers of `addDependencies()` func.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on a diff in pull request #3901: fix(#3896): Fix dependency inspector supporting property placeholders
Posted by GitBox <gi...@apache.org>.
christophd commented on code in PR #3901:
URL: https://github.com/apache/camel-k/pull/3901#discussion_r1048433923
##########
pkg/util/source/inspector.go:
##########
@@ -336,6 +336,11 @@ func (i *baseInspector) discoverKamelets(meta *Metadata) {
}
func (i *baseInspector) addDependencies(uri string, meta *Metadata, consumer bool) error {
+ if !strings.Contains(uri, ":") {
Review Comment:
the `DecodeComponent()` logic requires a scheme and a resource path part otherwise it will return nil (see https://github.com/apache/camel-k/blob/main/pkg/util/camel/camel_runtime_catalog.go#L185)
so the uri you have mentioned with just `mycomponent` will fail with the new error `component not found for uri ...`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on pull request #3901: fix(#3896): Fix dependency inspector supporting property placeholders
Posted by GitBox <gi...@apache.org>.
christophd commented on PR #3901:
URL: https://github.com/apache/camel-k/pull/3901#issuecomment-1353139203
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] tadayosi commented on pull request #3901: fix(#3896): Fix dependency inspector supporting property placeholders
Posted by GitBox <gi...@apache.org>.
tadayosi commented on PR #3901:
URL: https://github.com/apache/camel-k/pull/3901#issuecomment-1354310169
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] tadayosi commented on a diff in pull request #3901: fix(#3896): Fix dependency inspector supporting property placeholders
Posted by GitBox <gi...@apache.org>.
tadayosi commented on code in PR #3901:
URL: https://github.com/apache/camel-k/pull/3901#discussion_r1049174496
##########
pkg/util/source/inspector.go:
##########
@@ -336,6 +336,11 @@ func (i *baseInspector) discoverKamelets(meta *Metadata) {
}
func (i *baseInspector) addDependencies(uri string, meta *Metadata, consumer bool) error {
+ if !strings.Contains(uri, ":") {
+ // given URI is not in required format (maybe using a property placeholder such as {{url}})
+ return nil
+ }
+
candidateComp, scheme := i.catalog.DecodeComponent(uri)
if candidateComp == nil || scheme == nil {
Review Comment:
here
##########
pkg/util/source/inspector.go:
##########
@@ -336,6 +336,11 @@ func (i *baseInspector) discoverKamelets(meta *Metadata) {
}
func (i *baseInspector) addDependencies(uri string, meta *Metadata, consumer bool) error {
+ if !strings.Contains(uri, ":") {
Review Comment:
As Pasquale already pointed out, it appeares this check should be smarter. Example:
https://github.com/apache/camel-kamelets/blob/main/kamelets/elasticsearch-search-source.kamelet.yaml#L110
My suggestion would be, instead of checking the uri at the beginning of this func, what about examining the `scheme` and `candidateComp` values inside the block at line 345 after calling `DecodeComponent()` method?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] tadayosi commented on pull request #3901: fix(#3896): Fix dependency inspector supporting property placeholders
Posted by GitBox <gi...@apache.org>.
tadayosi commented on PR #3901:
URL: https://github.com/apache/camel-k/pull/3901#issuecomment-1352516553
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on pull request #3901: fix(#3896): Fix dependency inspector supporting property placeholders
Posted by GitBox <gi...@apache.org>.
christophd commented on PR #3901:
URL: https://github.com/apache/camel-k/pull/3901#issuecomment-1375798210
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] tadayosi commented on pull request #3901: fix(#3896): Fix dependency inspector supporting property placeholders
Posted by GitBox <gi...@apache.org>.
tadayosi commented on PR #3901:
URL: https://github.com/apache/camel-k/pull/3901#issuecomment-1352510771
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on pull request #3901: fix(#3896): Fix dependency inspector supporting property placeholders
Posted by GitBox <gi...@apache.org>.
christophd commented on PR #3901:
URL: https://github.com/apache/camel-k/pull/3901#issuecomment-1350724753
FYI @tadayosi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [camel-k] christophd commented on a diff in pull request #3901: fix(#3896): Fix dependency inspector supporting property placeholders
Posted by GitBox <gi...@apache.org>.
christophd commented on code in PR #3901:
URL: https://github.com/apache/camel-k/pull/3901#discussion_r1048450457
##########
pkg/util/source/inspector.go:
##########
@@ -336,6 +336,11 @@ func (i *baseInspector) discoverKamelets(meta *Metadata) {
}
func (i *baseInspector) addDependencies(uri string, meta *Metadata, consumer bool) error {
+ if !strings.Contains(uri, ":") {
Review Comment:
this has always been the logic but now we use to raise errors when there is a uri not matching the format `scheme:resourcepath`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org