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
