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/02 02:36:44 UTC
[GitHub] [camel-k] claudio4j opened a new pull request, #3870: fix(service): Create normal service when knative-service trait is disabled (1.10.x)
claudio4j opened a new pull request, #3870:
URL: https://github.com/apache/camel-k/pull/3870
https://github.com/apache/camel-k/issues/3849
When knative-service and k8s service traits are enabled, the priority is to use knative-service in knative profile
(cherry picked from commit 07563546a5d04c53dab122e90bc768e08514defe)
**Release Note**
```release-note
NONE
```
--
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] claudio4j commented on pull request #3870: fix(service): Create normal service when knative-service trait is disabled (1.10.x)
Posted by GitBox <gi...@apache.org>.
claudio4j commented on PR #3870:
URL: https://github.com/apache/camel-k/pull/3870#issuecomment-1334690333
@christophd for review
--
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] claudio4j merged pull request #3870: fix(service): Create k8s service when knative-service trait is disabled (1.10.x)
Posted by GitBox <gi...@apache.org>.
claudio4j merged PR #3870:
URL: https://github.com/apache/camel-k/pull/3870
--
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 #3870: fix(service): Create k8s service when knative-service trait is disabled (1.10.x)
Posted by GitBox <gi...@apache.org>.
squakez commented on code in PR #3870:
URL: https://github.com/apache/camel-k/pull/3870#discussion_r1037891632
##########
pkg/trait/service.go:
##########
@@ -44,7 +44,7 @@ func newServiceTrait() Trait {
// IsAllowedInProfile overrides default.
func (t *serviceTrait) IsAllowedInProfile(profile v1.TraitProfile) bool {
return profile.Equal(v1.TraitProfileKubernetes) ||
Review Comment:
This is always `true` then. We should remove the func.
##########
pkg/trait/service.go:
##########
@@ -61,6 +61,15 @@ func (t *serviceTrait) Configure(e *Environment) (bool, error) {
return false, nil
}
+ // in case the knative-service and service trait are enabled, the knative-service has priority
+ // then this service is disabled
+ if e.GetTrait(knativeServiceTraitID) != nil {
Review Comment:
Have you considered any other place where to provide this check? I mean, it would be nice not to have dependency between traits (if it is possible, of course).
--
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 #3870: fix(service): Create k8s service when knative-service trait is disabled (1.10.x)
Posted by GitBox <gi...@apache.org>.
christophd commented on code in PR #3870:
URL: https://github.com/apache/camel-k/pull/3870#discussion_r1037873038
##########
e2e/global/knative/knative_test.go:
##########
@@ -107,6 +107,20 @@ func TestKnative(t *testing.T) {
Expect(Kamel("delete", "--all", "-n", ns).Execute()).To(Succeed())
})
+
+ t.Run("Knative-service disabled", func(t *testing.T) {
+ Expect(KamelRunWithID(operatorID, ns, "files/http_out.groovy", "-t", "knative-service.enabled=false").Execute()).To(Succeed())
+ Eventually(IntegrationPodPhase(ns, "http-out"), TestTimeoutLong).Should(Equal(v1.PodRunning))
+ Eventually(Service(ns, "http-out"), TestTimeoutShort).ShouldNot(BeNil())
Review Comment:
Maybe add a check that the KnativeService `http-out` is not present
##########
e2e/global/knative/knative_test.go:
##########
@@ -107,6 +107,20 @@ func TestKnative(t *testing.T) {
Expect(Kamel("delete", "--all", "-n", ns).Execute()).To(Succeed())
})
+
+ t.Run("Knative-service disabled", func(t *testing.T) {
+ Expect(KamelRunWithID(operatorID, ns, "files/http_out.groovy", "-t", "knative-service.enabled=false").Execute()).To(Succeed())
+ Eventually(IntegrationPodPhase(ns, "http-out"), TestTimeoutLong).Should(Equal(v1.PodRunning))
+ Eventually(Service(ns, "http-out"), TestTimeoutShort).ShouldNot(BeNil())
+ Expect(Kamel("delete", "--all", "-n", ns).Execute()).To(Succeed())
+ })
+
+ t.Run("Knative-service priority", func(t *testing.T) {
+ Expect(KamelRunWithID(operatorID, ns, "files/http_out.groovy").Execute()).To(Succeed())
+ Eventually(IntegrationPodPhase(ns, "http-out"), TestTimeoutLong).Should(Equal(v1.PodRunning))
+ Eventually(KnativeService(ns, "http-out"), TestTimeoutShort).ShouldNot(BeNil())
Review Comment:
Maybe add a check that the "normal" service `http-out` is not present
--
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 #3870: fix(service): Create k8s service when knative-service trait is disabled (1.10.x)
Posted by GitBox <gi...@apache.org>.
christophd commented on code in PR #3870:
URL: https://github.com/apache/camel-k/pull/3870#discussion_r1037902135
##########
pkg/trait/service.go:
##########
@@ -61,6 +61,15 @@ func (t *serviceTrait) Configure(e *Environment) (bool, error) {
return false, nil
}
+ // in case the knative-service and service trait are enabled, the knative-service has priority
+ // then this service is disabled
+ if e.GetTrait(knativeServiceTraitID) != nil {
Review Comment:
This PR is only a short-term solution. In long-term we should refactor the service traits and how they are configured and theirs dependency to each other.
The idea would be to configure all service traits with one single trait config and have a new `service.type` trait config property that selects the service nature (default vs. knative vs. any other)
--
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