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 2023/03/23 11:05:34 UTC
[GitHub] [camel-k] squakez opened a new pull request, #4153: fix(builder): native from source should rebuild
squakez opened a new pull request, #4153:
URL: https://github.com/apache/camel-k/pull/4153
<!-- Description -->
With this PR we will force a rebuild for Quarkus native for those languages that requires the source at build time.
Closes #4126
<!--
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(builder): native from source should rebuild
```
--
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] essobedo commented on a diff in pull request #4153: fix(builder): native from source should rebuild
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on code in PR #4153:
URL: https://github.com/apache/camel-k/pull/4153#discussion_r1146032181
##########
pkg/controller/integration/kits.go:
##########
@@ -250,3 +256,17 @@ func matchesTrait(it map[string]interface{}, kt map[string]interface{}) bool {
// perform exact match on the two trait maps
return reflect.DeepEqual(it, kt)
}
+
+func hasMatchingSources(it *v1.Integration, kit *v1.IntegrationKit) bool {
+ for _, itSource := range it.Sources() {
+ for _, ikSource := range kit.Spec.Sources {
+ if itSource.Content == ikSource.Content {
+ // found, let's move to the next one
+ break
+ }
+ return false
+ }
+
+ }
+ return true
Review Comment:
If the integration has more sources than the kit and it includes all the sources of the kit, this will run `true`, I'm not sure if it is expected, is-it?
##########
e2e/native/native_with_sources_test.go:
##########
@@ -54,9 +54,38 @@ func TestNativeHighMemoryIntegrations(t *testing.T) {
Should(WithTransform(getContainerCommand(), MatchRegexp(".*camel-k-integration-\\d+\\.\\d+\\.\\d+[-A-Za-z]*-runner.*")))
Eventually(IntegrationConditionStatus(ns, name, v1.IntegrationConditionReady), TestTimeoutShort).
Should(Equal(corev1.ConditionTrue))
+ Eventually(IntegrationLogs(ns, name), TestTimeoutShort).Should(ContainSubstring("Java Magicstring!"))
+ })
+
+ t.Run("java native same should not rebuild", func(t *testing.T) {
+ name := "java-native-clone"
+ Expect(KamelRunWithID(operatorID, ns, "files/Java.java", "--name", name,
+ "-t", "quarkus.package-type=native",
+ ).Execute()).To(Succeed())
+ // This one should run quickly as it suppose to reuse an IntegrationKit
+ Eventually(IntegrationPodPhase(ns, name), TestTimeoutShort).Should(Equal(corev1.PodRunning))
+ Eventually(IntegrationPod(ns, name), TestTimeoutShort).
+ Should(WithTransform(getContainerCommand(), MatchRegexp(".*camel-k-integration-\\d+\\.\\d+\\.\\d+[-A-Za-z]*-runner.*")))
+ Eventually(IntegrationConditionStatus(ns, name, v1.IntegrationConditionReady), TestTimeoutShort).
+ Should(Equal(corev1.ConditionTrue))
Eventually(IntegrationLogs(ns, name), TestTimeoutShort).Should(ContainSubstring("Java Magicstring!"))
+ Expect(IntegrationKit(ns, "java-native")).Should(Equal(IntegrationKit(ns, "java-native-clone")))
+ })
Review Comment:
Maybe this test should be included in the previous one as they are highly coupled, I mean if we launch only this test, it cannot pass.
##########
e2e/native/native_with_sources_test.go:
##########
@@ -54,9 +54,38 @@ func TestNativeHighMemoryIntegrations(t *testing.T) {
Should(WithTransform(getContainerCommand(), MatchRegexp(".*camel-k-integration-\\d+\\.\\d+\\.\\d+[-A-Za-z]*-runner.*")))
Eventually(IntegrationConditionStatus(ns, name, v1.IntegrationConditionReady), TestTimeoutShort).
Should(Equal(corev1.ConditionTrue))
+ Eventually(IntegrationLogs(ns, name), TestTimeoutShort).Should(ContainSubstring("Java Magicstring!"))
+ })
+
+ t.Run("java native same should not rebuild", func(t *testing.T) {
+ name := "java-native-clone"
+ Expect(KamelRunWithID(operatorID, ns, "files/Java.java", "--name", name,
+ "-t", "quarkus.package-type=native",
+ ).Execute()).To(Succeed())
+ // This one should run quickly as it suppose to reuse an IntegrationKit
+ Eventually(IntegrationPodPhase(ns, name), TestTimeoutShort).Should(Equal(corev1.PodRunning))
+ Eventually(IntegrationPod(ns, name), TestTimeoutShort).
+ Should(WithTransform(getContainerCommand(), MatchRegexp(".*camel-k-integration-\\d+\\.\\d+\\.\\d+[-A-Za-z]*-runner.*")))
+ Eventually(IntegrationConditionStatus(ns, name, v1.IntegrationConditionReady), TestTimeoutShort).
+ Should(Equal(corev1.ConditionTrue))
Eventually(IntegrationLogs(ns, name), TestTimeoutShort).Should(ContainSubstring("Java Magicstring!"))
+ Expect(IntegrationKit(ns, "java-native")).Should(Equal(IntegrationKit(ns, "java-native-clone")))
+ })
+ t.Run("java native should rebuild", func(t *testing.T) {
+ name := "java-native-2"
Review Comment:
ditto
--
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 #4153: fix(builder): native from source should rebuild
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #4153:
URL: https://github.com/apache/camel-k/pull/4153#issuecomment-1480993101
fyi @essobedo @mertdotcc
--
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 #4153: fix(builder): native from source should rebuild
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #4153:
URL: https://github.com/apache/camel-k/pull/4153#discussion_r1146283684
##########
pkg/controller/integration/kits.go:
##########
@@ -250,3 +256,17 @@ func matchesTrait(it map[string]interface{}, kt map[string]interface{}) bool {
// perform exact match on the two trait maps
return reflect.DeepEqual(it, kt)
}
+
+func hasMatchingSources(it *v1.Integration, kit *v1.IntegrationKit) bool {
+ for _, itSource := range it.Sources() {
+ for _, ikSource := range kit.Spec.Sources {
+ if itSource.Content == ikSource.Content {
+ // found, let's move to the next one
+ break
+ }
+ return false
+ }
+
+ }
+ return true
Review Comment:
Yeah, probably makes sense to only match this when the number of sources matches 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] squakez commented on a diff in pull request #4153: fix(builder): native from source should rebuild
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #4153:
URL: https://github.com/apache/camel-k/pull/4153#discussion_r1146051473
##########
pkg/controller/integration/kits.go:
##########
@@ -250,3 +256,17 @@ func matchesTrait(it map[string]interface{}, kt map[string]interface{}) bool {
// perform exact match on the two trait maps
return reflect.DeepEqual(it, kt)
}
+
+func hasMatchingSources(it *v1.Integration, kit *v1.IntegrationKit) bool {
+ for _, itSource := range it.Sources() {
+ for _, ikSource := range kit.Spec.Sources {
+ if itSource.Content == ikSource.Content {
+ // found, let's move to the next one
+ break
+ }
+ return false
+ }
+
+ }
+ return true
Review Comment:
No, it is the other way around. If the integration has more sources than the kit, it will fail finding all of them equals in the kit. If the kit has more sources, then it's still okey as it may hold more tolerated by the integration. However this is a scenario which should never happen as in the native case (which is the only one where we include integration kit sources) we always have a single source in Integration and IntegrationKit.
--
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] mertdotcc commented on pull request #4153: fix(builder): native from source should rebuild
Posted by "mertdotcc (via GitHub)" <gi...@apache.org>.
mertdotcc commented on PR #4153:
URL: https://github.com/apache/camel-k/pull/4153#issuecomment-1481027697
@squakez Is it possible to release this fix with a minor version? `1.12.1`?
--
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 #4153: fix(builder): native from source should rebuild
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on PR #4153:
URL: https://github.com/apache/camel-k/pull/4153#issuecomment-1481035090
> @squakez Is it possible to release this fix with a minor version? `1.12.1`?
Yes, this is more important for 1.12 at this stage. But the workflow is we discuss the PR on `main` and once it gets merged we do the required backport. We will understand if and when to release a minor for 1.12.1 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] squakez merged pull request #4153: fix(builder): native from source should rebuild
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez merged PR #4153:
URL: https://github.com/apache/camel-k/pull/4153
--
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 #4153: fix(builder): native from source should rebuild
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #4153:
URL: https://github.com/apache/camel-k/pull/4153#discussion_r1146052401
##########
e2e/native/native_with_sources_test.go:
##########
@@ -54,9 +54,38 @@ func TestNativeHighMemoryIntegrations(t *testing.T) {
Should(WithTransform(getContainerCommand(), MatchRegexp(".*camel-k-integration-\\d+\\.\\d+\\.\\d+[-A-Za-z]*-runner.*")))
Eventually(IntegrationConditionStatus(ns, name, v1.IntegrationConditionReady), TestTimeoutShort).
Should(Equal(corev1.ConditionTrue))
+ Eventually(IntegrationLogs(ns, name), TestTimeoutShort).Should(ContainSubstring("Java Magicstring!"))
+ })
+
+ t.Run("java native same should not rebuild", func(t *testing.T) {
+ name := "java-native-clone"
+ Expect(KamelRunWithID(operatorID, ns, "files/Java.java", "--name", name,
+ "-t", "quarkus.package-type=native",
+ ).Execute()).To(Succeed())
+ // This one should run quickly as it suppose to reuse an IntegrationKit
+ Eventually(IntegrationPodPhase(ns, name), TestTimeoutShort).Should(Equal(corev1.PodRunning))
+ Eventually(IntegrationPod(ns, name), TestTimeoutShort).
+ Should(WithTransform(getContainerCommand(), MatchRegexp(".*camel-k-integration-\\d+\\.\\d+\\.\\d+[-A-Za-z]*-runner.*")))
+ Eventually(IntegrationConditionStatus(ns, name, v1.IntegrationConditionReady), TestTimeoutShort).
+ Should(Equal(corev1.ConditionTrue))
Eventually(IntegrationLogs(ns, name), TestTimeoutShort).Should(ContainSubstring("Java Magicstring!"))
+ Expect(IntegrationKit(ns, "java-native")).Should(Equal(IntegrationKit(ns, "java-native-clone")))
+ })
Review Comment:
Yeah, it could be nested under the other one.
--
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] essobedo commented on a diff in pull request #4153: fix(builder): native from source should rebuild
Posted by "essobedo (via GitHub)" <gi...@apache.org>.
essobedo commented on code in PR #4153:
URL: https://github.com/apache/camel-k/pull/4153#discussion_r1146150423
##########
pkg/controller/integration/kits.go:
##########
@@ -250,3 +256,17 @@ func matchesTrait(it map[string]interface{}, kt map[string]interface{}) bool {
// perform exact match on the two trait maps
return reflect.DeepEqual(it, kt)
}
+
+func hasMatchingSources(it *v1.Integration, kit *v1.IntegrationKit) bool {
+ for _, itSource := range it.Sources() {
+ for _, ikSource := range kit.Spec.Sources {
+ if itSource.Content == ikSource.Content {
+ // found, let's move to the next one
+ break
+ }
+ return false
+ }
+
+ }
+ return true
Review Comment:
Why not comparing also the size of the slices?
--
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 #4153: fix(builder): native from source should rebuild
Posted by "squakez (via GitHub)" <gi...@apache.org>.
squakez commented on code in PR #4153:
URL: https://github.com/apache/camel-k/pull/4153#discussion_r1146084126
##########
pkg/controller/integration/kits.go:
##########
@@ -250,3 +256,17 @@ func matchesTrait(it map[string]interface{}, kt map[string]interface{}) bool {
// perform exact match on the two trait maps
return reflect.DeepEqual(it, kt)
}
+
+func hasMatchingSources(it *v1.Integration, kit *v1.IntegrationKit) bool {
+ for _, itSource := range it.Sources() {
+ for _, ikSource := range kit.Spec.Sources {
+ if itSource.Content == ikSource.Content {
+ // found, let's move to the next one
+ break
+ }
+ return false
+ }
+
+ }
+ return true
Review Comment:
I refactored the loop anyhow to validate against all possible scenarios.
--
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