Re: [PR] fix(#5402): Evaluate Knative profile based on Serving/Eventing installed [camel-k]
christophd merged PR #5419: URL: https://github.com/apache/camel-k/pull/5419 -- 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(#5402): Evaluate Knative profile based on Serving/Eventing installed [camel-k]
squakez commented on code in PR #5419:
URL: https://github.com/apache/camel-k/pull/5419#discussion_r1580833131
##
pkg/util/knative/knative.go:
##
@@ -75,7 +75,27 @@ func CreateSubscription(channelReference
corev1.ObjectReference, serviceName str
}
}
-func CreateTrigger(brokerReference corev1.ObjectReference, serviceName string,
eventType string, path string) *eventing.Trigger {
+// CreateServiceTrigger create Knative trigger with arbitrary Kubernetes
Service as a subscriber - usually used when no Knative Serving is available on
the cluster.
+func CreateServiceTrigger(brokerReference corev1.ObjectReference, serviceName
string, eventType string, path string) (*eventing.Trigger, error) {
+ subscriberRef := duckv1.KReference{
+ APIVersion: "v1",
+ Kind: "Service",
+ Name: serviceName,
+ }
+ return CreateTrigger(brokerReference, subscriberRef, eventType, path)
+}
+
+// CreateKnativeServiceTrigger create Knative trigger with Knative Serving
Service as a subscriber - default option when Knative Serving is available on
the cluster.
+func CreateKnativeServiceTrigger(brokerReference corev1.ObjectReference,
serviceName string, eventType string, path string) (*eventing.Trigger, error) {
+ subscriberRef := duckv1.KReference{
+ APIVersion: serving.SchemeGroupVersion.String(),
+ Kind: "Service",
+ Name: serviceName,
+ }
+ return CreateTrigger(brokerReference, subscriberRef, eventType, path)
+}
+
+func CreateTrigger(brokerReference corev1.ObjectReference, subscriberRef
duckv1.KReference, eventType string, path string) (*eventing.Trigger, error) {
Review Comment:
I think it makes sense to have this with a private scope as I think any
client has to call either Service or KnativeService on purpose.
--
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(#5402): Evaluate Knative profile based on Serving/Eventing installed [camel-k]
christophd commented on code in PR #5419:
URL: https://github.com/apache/camel-k/pull/5419#discussion_r1580743874
##
pkg/trait/knative.go:
##
@@ -560,3 +571,19 @@ func (t *knativeTrait) extractServices(names []string,
serviceType knativeapi.Ca
sort.Strings(answer)
return answer
}
+
+func (t *knativeTrait) determineServiceType(e *Environment) (string, error) {
+ controllerStrategy, err := e.DetermineControllerStrategy()
+ if err != nil {
+ return "", err
+ }
+
+ switch controllerStrategy {
+ case ControllerStrategyKnativeService:
+ return serving.SchemeGroupVersion.String(), nil
+ case ControllerStrategyDeployment:
Review Comment:
I' d rather raise the error in this case because a CronJob deployment
strategy does not fit the Trigger.
--
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(#5402): Evaluate Knative profile based on Serving/Eventing installed [camel-k]
squakez commented on code in PR #5419:
URL: https://github.com/apache/camel-k/pull/5419#discussion_r1580698963
##
pkg/trait/knative.go:
##
@@ -486,9 +488,18 @@ func (t *knativeTrait) createTrigger(e *Environment, ref
*corev1.ObjectReference
if ref.Namespace == "" {
ref.Namespace = e.Integration.Namespace
}
- trigger := knativeutil.CreateTrigger(*ref, e.Integration.Name,
eventType, path)
+ serviceAPIVersion, err := t.determineServiceType(e)
+ if err != nil {
+ return err
+ }
+ trigger, err := knativeutil.CreateTrigger(*ref,
e.Integration.Name, serviceAPIVersion, eventType, path)
Review Comment:
In order to simplify the maintenance, see the some comment below related to
the CreateTrigger func. I think here, we should just call either
CreateTriggerService or CreateTriggerKnativeService to make it more explicit.
Also mind that we do have CronJob controller strategy.
##
pkg/trait/knative.go:
##
@@ -560,3 +571,19 @@ func (t *knativeTrait) extractServices(names []string,
serviceType knativeapi.Ca
sort.Strings(answer)
return answer
}
+
+func (t *knativeTrait) determineServiceType(e *Environment) (string, error) {
+ controllerStrategy, err := e.DetermineControllerStrategy()
+ if err != nil {
+ return "", err
+ }
+
+ switch controllerStrategy {
+ case ControllerStrategyKnativeService:
+ return serving.SchemeGroupVersion.String(), nil
+ case ControllerStrategyDeployment:
Review Comment:
Probably also when CronJob strategy. In fact, why not defaulting to
CreateTriggerService if not Knative enabled?
##
pkg/util/knative/knative.go:
##
@@ -75,7 +75,7 @@ func CreateSubscription(channelReference
corev1.ObjectReference, serviceName str
}
}
-func CreateTrigger(brokerReference corev1.ObjectReference, serviceName string,
eventType string, path string) *eventing.Trigger {
+func CreateTrigger(brokerReference corev1.ObjectReference, serviceName string,
serviceAPIVersion string, eventType string, path string) (*eventing.Trigger,
error) {
Review Comment:
Altough the logic is correct, I think we're making implicit an
implementation detail that should be made explicit making the long term
maintenance a possible problem (above all for anybody not knowing this part of
the project). I think it's okey to refactor the code and have a private
`createTrigger` func like this one, and then two public
`CreateTriggerService()` and `CreateTriggerKnativeService` which clarify the
intention in the signature. These two func would be calling `createTrigger`
with the exepected apiVersion.
--
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(#5402): Evaluate Knative profile based on Serving/Eventing installed [camel-k]
christophd commented on PR #5419: URL: https://github.com/apache/camel-k/pull/5419#issuecomment-2078890768 @squakez issues are fixed now. also added some unit tests. ready for review, 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
