You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@camel.apache.org by "squakez (via GitHub)" <gi...@apache.org> on 2024/03/07 16:38:35 UTC
[PR] fix(traits): use Comparable matches [camel-k]
squakez opened a new pull request, #5230:
URL: https://github.com/apache/camel-k/pull/5230
Reverting #4512 which introduced a function diverging the match from the original design
This PR aims to fix a diverging design I introduced some times ago. With this one we move back to the original design making correct usage of `Comparable` interface in trait which is used by the operator to distinguish when to reuse or rebuild an Integration based only on certain IntegrationKit traits changes (ie, runtime version, builder properties).
<!-- Description -->
<!--
Enter your extended release note in the below block. If the PR requires
additional action from users switching to the new release, include the string
"action required". If no release note is required, write "NONE".
You can (optionally) mark this PR with labels "kind/bug" or "kind/feature" to make sure
the text is added to the right section of the release notes.
-->
**Release Note**
```release-note
fix(traits): use Comparable matches
```
--
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
Re: [PR] fix(traits): use Comparable matches [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
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
<string>: 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
<string>: 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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
Posted by "lburgazzoli (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
Posted by "squakez (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(traits): use Comparable matches [camel-k]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
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-unsubscribe@camel.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org