wmedvede commented on code in PR #350:
URL:
https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/350#discussion_r1460383185
##########
controllers/profiles/common/object_creators.go:
##########
@@ -197,6 +211,85 @@ func ServiceCreator(workflow *operatorapi.SonataFlow)
(client.Object, error) {
return service, nil
}
+// SinkBindingCreator is an ObjectsCreator for SinkBinding.
+// It will create v1.SinkBinding based on events defined in workflow.
+func SinkBindingCreator(workflow *operatorapi.SonataFlow) (client.Object,
error) {
+ lbl := workflowproj.GetDefaultLabels(workflow)
+
+ // skip if no produced event is found
+ if workflow.Spec.Sink == nil ||
!workflowdef.ContainsEventKind(workflow, cncfmodel.EventKindProduced) {
+ return nil, nil
+ }
+
+ sink := workflow.Spec.Sink
+
+ // subject must be deployment to inject K_SINK, service won't work
+ sinkBinding := &sourcesv1.SinkBinding{
+ ObjectMeta: metav1.ObjectMeta{
+ Name: strings.ToLower(fmt.Sprintf("sb-%s",
workflow.Name)),
Review Comment:
Maybe we can do here a naming similar to the one used for the trigger:
"%s-sb", workflow name first.
I know that in kogito-runtimes we are right now creating names like
sb-my-workflow, but, I think that for the operator we can align, since we have
for instance, my-worfklow-props, my-workflow-event1-trigger, and so on. So why
not to have my-wofkflow-sb....
##########
controllers/discovery/discovery.go:
##########
@@ -80,35 +80,34 @@ type ServiceCatalog interface {
Query(ctx context.Context, uri ResourceUri, outputFormat string)
(string, error)
}
-type sonataFlowServiceCatalog struct {
+type SonataFlowServiceCatalog struct {
Review Comment:
why do we need this change?
##########
controllers/profiles/common/ensurer.go:
##########
@@ -66,22 +67,10 @@ func (d *defaultObjectEnsurer) Ensure(ctx context.Context,
workflow *operatorapi
result := controllerutil.OperationResultNone
object, err := d.creator(workflow)
- if err != nil {
+ if err != nil || object == nil {
return nil, result, err
Review Comment:
If we have an error here, then we return
##########
controllers/profiles/common/knative.go:
##########
@@ -0,0 +1,75 @@
+// Copyright 2024 Apache Software Foundation (ASF)
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package common
+
+import (
+ "context"
+
+ operatorapi
"github.com/apache/incubator-kie-kogito-serverless-operator/api/v1alpha08"
+
"github.com/apache/incubator-kie-kogito-serverless-operator/controllers/discovery"
+ "github.com/apache/incubator-kie-kogito-serverless-operator/log"
+ "k8s.io/klog/v2"
+ "sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+var _ KnativeEventingHandlerInterface = &knativeObjectManager{}
+
+type knativeObjectManager struct {
+ sinkBinding ObjectEnsurer
+ trigger ObjectsEnsurer
+ *StateSupport
+}
+
+func KnativeEventingHandler(support *StateSupport)
KnativeEventingHandlerInterface {
+ return &knativeObjectManager{
+ sinkBinding: NewObjectEnsurer(support.C, SinkBindingCreator),
+ trigger: NewObjectsEnsurer(support.C, TriggersCreator),
+ StateSupport: support,
+ }
+}
+
+type KnativeEventingHandlerInterface interface {
Review Comment:
I believe we can name this interface just KnativeEventingHandler
##########
controllers/profiles/common/ensurer.go:
##########
@@ -66,22 +67,10 @@ func (d *defaultObjectEnsurer) Ensure(ctx context.Context,
workflow *operatorapi
result := controllerutil.OperationResultNone
object, err := d.creator(workflow)
- if err != nil {
+ if err != nil || object == nil {
return nil, result, err
}
- if result, err = controllerutil.CreateOrPatch(ctx, d.c, object,
- func() error {
- for _, v := range visitors {
- if visitorErr := v(object)(); visitorErr != nil
{
- return visitorErr
- }
- }
- return controllerutil.SetControllerReference(workflow,
object, d.c.Scheme())
- }); err != nil {
- return nil, result, err
- }
- klog.V(log.I).InfoS("Object operation finalized", "result", result,
"kind", object.GetObjectKind().GroupVersionKind().String(), "name",
object.GetName(), "namespace", object.GetNamespace())
- return object, result, nil
+ return ensureObject(ctx, workflow, visitors, result, err, d.c, object)
Review Comment:
I believe that here we don't need to pass any error, see the comment above.
##########
controllers/profiles/common/ensurer.go:
##########
@@ -97,3 +86,61 @@ func (d *noopObjectEnsurer) Ensure(ctx context.Context,
workflow *operatorapi.So
result := controllerutil.OperationResultNone
return nil, result, nil
}
+
+// ObjectsEnsurer is an ensurer to apply multiple objects
+type ObjectsEnsurer interface {
+ Ensure(ctx context.Context, workflow *operatorapi.SonataFlow, visitors
...MutateVisitor) []ObjectEnsurerResult
+}
+
+type ObjectEnsurerResult struct {
+ client.Object
+ Result controllerutil.OperationResult
+ Error error
+}
+
+func NewObjectsEnsurer(client client.Client, creator ObjectsCreator)
ObjectsEnsurer {
+ return &defaultObjectsEnsurer{
+ c: client,
+ creator: creator,
+ }
+}
+
+type defaultObjectsEnsurer struct {
+ ObjectsEnsurer
+ c client.Client
+ creator ObjectsCreator
+}
+
+func (d *defaultObjectsEnsurer) Ensure(ctx context.Context, workflow
*operatorapi.SonataFlow, visitors ...MutateVisitor) []ObjectEnsurerResult {
+ result := controllerutil.OperationResultNone
+
+ objects, err := d.creator(workflow)
+ if err != nil {
+ return []ObjectEnsurerResult{{nil, result, err}}
+ }
+ var ensureResult []ObjectEnsurerResult
+ for _, object := range objects {
+ ensureObject, c, err := ensureObject(ctx, workflow, visitors,
result, err, d.c, object)
+ ensureResult = append(ensureResult,
ObjectEnsurerResult{ensureObject, c, err})
+ if err != nil {
+ return ensureResult
+ }
+ }
+ return ensureResult
+}
+
+func ensureObject(ctx context.Context, workflow *operatorapi.SonataFlow,
visitors []MutateVisitor, result controllerutil.OperationResult, err error, c
client.Client, object client.Object) (client.Object,
controllerutil.OperationResult, error) {
Review Comment:
see comments above, I think this method shouldn't receive any err parameter.
##########
controllers/profiles/common/knative.go:
##########
@@ -0,0 +1,75 @@
+// Copyright 2024 Apache Software Foundation (ASF)
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package common
+
+import (
+ "context"
+
+ operatorapi
"github.com/apache/incubator-kie-kogito-serverless-operator/api/v1alpha08"
+
"github.com/apache/incubator-kie-kogito-serverless-operator/controllers/discovery"
+ "github.com/apache/incubator-kie-kogito-serverless-operator/log"
+ "k8s.io/klog/v2"
+ "sigs.k8s.io/controller-runtime/pkg/client"
+)
+
+var _ KnativeEventingHandlerInterface = &knativeObjectManager{}
+
+type knativeObjectManager struct {
+ sinkBinding ObjectEnsurer
+ trigger ObjectsEnsurer
+ *StateSupport
+}
+
+func KnativeEventingHandler(support *StateSupport)
KnativeEventingHandlerInterface {
Review Comment:
If we rename the interface too KnativeEventingHandler (see comment below) ,
we can call this function NewKnativeEventingHandler
##########
controllers/discovery/discovery.go:
##########
@@ -80,35 +80,34 @@ type ServiceCatalog interface {
Query(ctx context.Context, uri ResourceUri, outputFormat string)
(string, error)
}
-type sonataFlowServiceCatalog struct {
+type SonataFlowServiceCatalog struct {
kubernetesCatalog ServiceCatalog
- knativeCatalog ServiceCatalog
+ KnativeCatalog ServiceCatalog
openshiftCatalog ServiceCatalog
}
// NewServiceCatalog returns a new ServiceCatalog configured to resolve
kubernetes, knative, and openshift resource addresses.
func NewServiceCatalog(cli client.Client, knDiscoveryClient
*KnDiscoveryClient, openShiftDiscoveryClient *OpenShiftDiscoveryClient)
ServiceCatalog {
- return &sonataFlowServiceCatalog{
+ return &SonataFlowServiceCatalog{
kubernetesCatalog: newK8SServiceCatalog(cli),
- knativeCatalog: newKnServiceCatalog(knDiscoveryClient),
+ KnativeCatalog: newKnServiceCatalog(knDiscoveryClient),
openshiftCatalog:
newOpenShiftServiceCatalog(openShiftDiscoveryClient),
}
}
func NewServiceCatalogForConfig(cli client.Client, cfg *rest.Config)
ServiceCatalog {
- return &sonataFlowServiceCatalog{
+ return &SonataFlowServiceCatalog{
kubernetesCatalog: newK8SServiceCatalog(cli),
- knativeCatalog: newKnServiceCatalogForConfig(cfg),
- openshiftCatalog:
newOpenShiftServiceCatalogForClientAndConfig(cli, cfg),
+ KnativeCatalog: newKnServiceCatalogForConfig(cfg),
}
}
-func (c *sonataFlowServiceCatalog) Query(ctx context.Context, uri ResourceUri,
outputFormat string) (string, error) {
+func (c *SonataFlowServiceCatalog) Query(ctx context.Context, uri ResourceUri,
outputFormat string) (string, error) {
Review Comment:
I don't think we need this change
##########
controllers/discovery/discovery.go:
##########
@@ -80,35 +80,34 @@ type ServiceCatalog interface {
Query(ctx context.Context, uri ResourceUri, outputFormat string)
(string, error)
}
-type sonataFlowServiceCatalog struct {
+type SonataFlowServiceCatalog struct {
kubernetesCatalog ServiceCatalog
- knativeCatalog ServiceCatalog
+ KnativeCatalog ServiceCatalog
Review Comment:
same, here, whay we need this change?
##########
controllers/profiles/common/constants/workflows.go:
##########
@@ -16,4 +16,10 @@ package constants
const (
MicroprofileServiceCatalogPropertyPrefix =
"org.kie.kogito.addons.discovery."
+ OutgoingEventsURL =
"mp.messaging.outgoing.kogito_outgoing_stream.url"
Review Comment:
I think this properties that has "kogito", we can refix with Kogito, kind of
KogitoOutoingEventsURL, I it helps to link the the kogito-runtime develoment.
##########
controllers/discovery/discovery.go:
##########
@@ -80,35 +80,34 @@ type ServiceCatalog interface {
Query(ctx context.Context, uri ResourceUri, outputFormat string)
(string, error)
}
-type sonataFlowServiceCatalog struct {
+type SonataFlowServiceCatalog struct {
kubernetesCatalog ServiceCatalog
- knativeCatalog ServiceCatalog
+ KnativeCatalog ServiceCatalog
openshiftCatalog ServiceCatalog
}
// NewServiceCatalog returns a new ServiceCatalog configured to resolve
kubernetes, knative, and openshift resource addresses.
func NewServiceCatalog(cli client.Client, knDiscoveryClient
*KnDiscoveryClient, openShiftDiscoveryClient *OpenShiftDiscoveryClient)
ServiceCatalog {
- return &sonataFlowServiceCatalog{
+ return &SonataFlowServiceCatalog{
kubernetesCatalog: newK8SServiceCatalog(cli),
- knativeCatalog: newKnServiceCatalog(knDiscoveryClient),
+ KnativeCatalog: newKnServiceCatalog(knDiscoveryClient),
openshiftCatalog:
newOpenShiftServiceCatalog(openShiftDiscoveryClient),
}
}
func NewServiceCatalogForConfig(cli client.Client, cfg *rest.Config)
ServiceCatalog {
- return &sonataFlowServiceCatalog{
+ return &SonataFlowServiceCatalog{
kubernetesCatalog: newK8SServiceCatalog(cli),
- knativeCatalog: newKnServiceCatalogForConfig(cfg),
- openshiftCatalog:
newOpenShiftServiceCatalogForClientAndConfig(cli, cfg),
Review Comment:
this openshiftCatalog can't be removed, or the OpenshitRelated
queries won't work annymore.
In general I believe that this file is good, in master, and I see breaking
changes here.
Why do we need this changes?
##########
controllers/profiles/common/properties/properties.go:
##########
@@ -0,0 +1,47 @@
+// Copyright 2024 Apache Software Foundation (ASF)
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package properties
+
+import (
+ operatorapi
"github.com/apache/incubator-kie-kogito-serverless-operator/api/v1alpha08"
+
"github.com/apache/incubator-kie-kogito-serverless-operator/controllers/profiles/common/constants"
+
"github.com/apache/incubator-kie-kogito-serverless-operator/controllers/workflowdef"
+ "github.com/magiconair/properties"
+ cncfmodel "github.com/serverlessworkflow/sdk-go/v2/model"
+)
+
+// GenerateKnativeEventingWorkflowProperties returns the set of application
properties required for the workflow to produce or consume
+// Knative Events. For the calculation this function considers if the Job
Service is present in the
Review Comment:
copy pasted description here "if the Job Service....".
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
