Re: [PR] fix(traits): use Comparable matches [camel-k]
squakez merged PR #5230: URL: https://github.com/apache/camel-k/pull/5230 -- 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-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
github-actions[bot] commented on PR #5230: URL: https://github.com/apache/camel-k/pull/5230#issuecomment-1990953463 :heavy_check_mark: Unit test coverage report - coverage increased from 36.9% to 37.1% (**+0.2%**) -- 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-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
github-actions[bot] commented on PR #5230: URL: https://github.com/apache/camel-k/pull/5230#issuecomment-1988654389 :heavy_check_mark: Unit test coverage report - coverage increased from 36.9% to 37.1% (**+0.2%**) -- 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-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
squakez commented on PR #5230: URL: https://github.com/apache/camel-k/pull/5230#issuecomment-1986387256 ``` --- PASS: TestNativeHighMemoryIntegrations (1750.79s) --- PASS: TestNativeHighMemoryIntegrations/java_native_support (847.92s) --- PASS: TestNativeHighMemoryIntegrations/java_native_support/java_native_same_should_not_rebuild (2.22s) --- PASS: TestNativeHighMemoryIntegrations/java_native_support/java_native_should_rebuild (417.19s) --- PASS: TestNativeHighMemoryIntegrations/groovy_native_support (457.62s) --- PASS: TestNativeHighMemoryIntegrations/kotlin_native_support (434.65s) PASS ok github.com/apache/camel-k/v2/e2e/native 1750.826s --- PASS: TestNativeBinding (435.95s) --- PASS: TestNativeBinding/binding_with_native_build (421.78s) --- PASS: TestNativeIntegrations (846.09s) --- PASS: TestNativeIntegrations/unsupported_integration_source_language (0.80s) --- PASS: TestNativeIntegrations/xml_native_support (410.86s) --- PASS: TestNativeIntegrations/automatic_rollout_deployment_from_jvm_to_native_kit (424.50s) --- PASS: TestNativeIntegrations/automatic_rollout_deployment_from_jvm_to_native_kit/yaml_native_should_not_rebuild (2.76s) PASS ok github.com/apache/camel-k/v2/e2e/native 1282.082s `` -- 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-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
github-actions[bot] commented on PR #5230: URL: https://github.com/apache/camel-k/pull/5230#issuecomment-1985981407 :heavy_check_mark: Unit test coverage report - coverage increased from 36.9% to 37.1% (**+0.2%**) -- 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-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
lburgazzoli commented on code in PR #5230:
URL: https://github.com/apache/camel-k/pull/5230#discussion_r1517349545
##
pkg/trait/builder.go:
##
@@ -56,9 +57,18 @@ func (t *builderTrait) InfluencesKit() bool {
return true
}
-// InfluencesBuild overrides base class method.
-func (t *builderTrait) InfluencesBuild(this, prev map[string]interface{}) bool
{
- return true
+func (t *builderTrait) Matches(trait Trait) bool {
+ otherTrait, ok := trait.(*builderTrait)
+ if !ok {
+ return false
+ }
+ if t.BaseImage != otherTrait.BaseImage || len(t.Properties) !=
len(otherTrait.Properties) {
+ return false
+ }
+ // More sofisticated check if len is the same. Sort and compare via
slices equal func.
+ slices.Sort(t.Properties)
+ slices.Sort(otherTrait.Properties)
Review Comment:
That would be a sensible option.
Another one, but would require some more work so I don't think it is worth
at this stage, could be to do something similar what it is done in the golang
standard library where there is often the possibility to pass a function for
equality, sorting, etc. like https://pkg.go.dev/slices#EqualFunc
--
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-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
squakez commented on code in PR #5230:
URL: https://github.com/apache/camel-k/pull/5230#discussion_r1517281977
##
pkg/trait/builder.go:
##
@@ -56,9 +57,18 @@ func (t *builderTrait) InfluencesKit() bool {
return true
}
-// InfluencesBuild overrides base class method.
-func (t *builderTrait) InfluencesBuild(this, prev map[string]interface{}) bool
{
- return true
+func (t *builderTrait) Matches(trait Trait) bool {
+ otherTrait, ok := trait.(*builderTrait)
+ if !ok {
+ return false
+ }
+ if t.BaseImage != otherTrait.BaseImage || len(t.Properties) !=
len(otherTrait.Properties) {
+ return false
+ }
+ // More sofisticated check if len is the same. Sort and compare via
slices equal func.
+ slices.Sort(t.Properties)
+ slices.Sort(otherTrait.Properties)
Review Comment:
Yes, I see. For the sake of making the thing in a clean way, I can copy the
array and sorting/comparing them. It should not be too heavy from a computation
pov and it will save us against remote (but still possible) inconsistencies.
--
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-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
lburgazzoli commented on code in PR #5230:
URL: https://github.com/apache/camel-k/pull/5230#discussion_r1517247695
##
pkg/trait/builder.go:
##
@@ -56,9 +57,18 @@ func (t *builderTrait) InfluencesKit() bool {
return true
}
-// InfluencesBuild overrides base class method.
-func (t *builderTrait) InfluencesBuild(this, prev map[string]interface{}) bool
{
- return true
+func (t *builderTrait) Matches(trait Trait) bool {
+ otherTrait, ok := trait.(*builderTrait)
+ if !ok {
+ return false
+ }
+ if t.BaseImage != otherTrait.BaseImage || len(t.Properties) !=
len(otherTrait.Properties) {
+ return false
+ }
+ // More sofisticated check if len is the same. Sort and compare via
slices equal func.
+ slices.Sort(t.Properties)
+ slices.Sort(otherTrait.Properties)
Review Comment:
The structure backing the two object is definitively different, my point was
more on the fact if `Matches` is or will be used concurrently, then it may
result in an undefined behavior.
Given the current PR I don't think it can happens but since the method is a
public method part of some of the traits, then it may be used also outside the
current scope and I did want to raise it to avoid falling in the same trap as
https://github.com/apache/camel-k/pull/5188#discussion_r1512280139
--
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-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
squakez commented on code in PR #5230:
URL: https://github.com/apache/camel-k/pull/5230#discussion_r1517210674
##
pkg/trait/builder.go:
##
@@ -56,9 +57,18 @@ func (t *builderTrait) InfluencesKit() bool {
return true
}
-// InfluencesBuild overrides base class method.
-func (t *builderTrait) InfluencesBuild(this, prev map[string]interface{}) bool
{
- return true
+func (t *builderTrait) Matches(trait Trait) bool {
+ otherTrait, ok := trait.(*builderTrait)
+ if !ok {
+ return false
+ }
+ if t.BaseImage != otherTrait.BaseImage || len(t.Properties) !=
len(otherTrait.Properties) {
+ return false
+ }
+ // More sofisticated check if len is the same. Sort and compare via
slices equal func.
+ slices.Sort(t.Properties)
+ slices.Sort(otherTrait.Properties)
Review Comment:
Yes, but in this case I don't think it's a problem. This func is not
directly used by the trait per se but is a supporting function used in the
`matchesComparableTrait` func. Here what we do is to create 2 brand new traits
which are only used for comparison, so, I tend to think they are not really
affecting the rest of the execution as they are 2 new objects:
https://github.com/apache/camel-k/pull/5230/files#diff-80f4821d91dcccf8c11c32962f56b41b7dcfccadeb6366df3cf49baa55188ba1R637-R658
I think the structure backing the two object is different. But I may be
wrong. If you feel we need to prove it I can try to develop a unit test to make
sure this is not happening. Thanks.
--
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-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
squakez commented on PR #5230: URL: https://github.com/apache/camel-k/pull/5230#issuecomment-1985096678 ``` === NAME TestNativeHighMemoryIntegrations native_with_sources_test.go:60: Timed out after 60.000s. Expected : 2024-03-07 17:04:07,602 INFO [org.apa.cam.k.Runtime] (main) Apache Camel K Runtime 3.2.3 2024-03-07 17:04:07,602 INFO [org.apa.cam.qua.cor.CamelBootstrapRecorder] (main) Bootstrap runtime: org.apache.camel.quarkus.main.CamelMainRuntime 2024-03-07 17:04:07,602 INFO [org.apa.cam.mai.MainSupport] (main) Apache Camel (Main) 4.0.2 is starting 2024-03-07 17:04:07,610 INFO [org.apa.cam.imp.eng.AbstractCamelContext] (main) Apache Camel 4.0.2 (camel-1) is starting 2024-03-07 17:04:07,610 INFO [org.apa.cam.imp.eng.AbstractCamelContext] (main) Routes startup (started:0) 2024-03-07 17:04:07,610 INFO [org.apa.cam.imp.eng.AbstractCamelContext] (main) Apache Camel 4.0.2 (camel-1) started in 0ms (build:0ms init:0ms start:0ms) 2024-03-07 17:04:07,612 INFO [io.quarkus] (main) camel-k-integration 2.3.0-SNAPSHOT native (powered by Quarkus 3.2.9.Final) started in 0.052s. Listening on: http://0.0.0.0:8080 2024-03-07 17:04:07,612 INFO [io.quarkus] (main) Profile prod activated. 2024-03-07 17:04:07,612 INFO [io.quarkus] (main) Installed features: [camel-bean, camel-cloudevents, camel-core, camel-java-joor-dsl, camel-k-core, camel-k-runtime, camel-knative, camel-kubernetes, camel-timer, cdi, kubernetes-client, smallrye-context-propagation, vertx] to contain substring : Java Magicstring! ``` Fixing this. -- 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-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
squakez commented on PR #5230: URL: https://github.com/apache/camel-k/pull/5230#issuecomment-1985096440 Result of Quarkus native integrations: ``` --- PASS: TestNativeBinding (549.82s) --- PASS: TestNativeBinding/binding_with_native_build (535.03s) --- PASS: TestNativeIntegrations (1114.16s) --- PASS: TestNativeIntegrations/unsupported_integration_source_language (0.90s) --- PASS: TestNativeIntegrations/xml_native_support (632.23s) --- PASS: TestNativeIntegrations/automatic_rollout_deployment_from_jvm_to_native_kit (472.05s) --- PASS: TestNativeIntegrations/automatic_rollout_deployment_from_jvm_to_native_kit/yaml_native_should_not_rebuild (2.20s) ``` -- 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-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
lburgazzoli commented on code in PR #5230:
URL: https://github.com/apache/camel-k/pull/5230#discussion_r1516882356
##
pkg/trait/builder.go:
##
@@ -56,9 +57,18 @@ func (t *builderTrait) InfluencesKit() bool {
return true
}
-// InfluencesBuild overrides base class method.
-func (t *builderTrait) InfluencesBuild(this, prev map[string]interface{}) bool
{
- return true
+func (t *builderTrait) Matches(trait Trait) bool {
+ otherTrait, ok := trait.(*builderTrait)
+ if !ok {
+ return false
+ }
+ if t.BaseImage != otherTrait.BaseImage || len(t.Properties) !=
len(otherTrait.Properties) {
+ return false
+ }
+ // More sofisticated check if len is the same. Sort and compare via
slices equal func.
+ slices.Sort(t.Properties)
+ slices.Sort(otherTrait.Properties)
Review Comment:
nitpick: this method has a side effect as it alters the order of both the
current and the other trait properties.
--
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-unsubscr...@camel.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
github-actions[bot] commented on PR #5230: URL: https://github.com/apache/camel-k/pull/5230#issuecomment-1983979304 :heavy_check_mark: Unit test coverage report - coverage increased from 36.8% to 37% (**+0.2%**) -- 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-unsubscr...@camel.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
