ricardozanini commented on code in PR #367:
URL:
https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/367#discussion_r1467724776
##########
controllers/profiles/dev/states_dev.go:
##########
@@ -62,23 +62,26 @@ func (e *ensureRunningWorkflowState) CanReconcile(workflow
*operatorapi.SonataFl
func (e *ensureRunningWorkflowState) Do(ctx context.Context, workflow
*operatorapi.SonataFlow) (ctrl.Result, []client.Object, error) {
var objs []client.Object
- flowDefCM, _, err := e.ensurers.definitionConfigMap.Ensure(ctx,
workflow, ensureWorkflowDefConfigMapMutator(workflow))
- if err != nil {
- return ctrl.Result{Requeue: false}, objs, err
- }
- objs = append(objs, flowDefCM)
-
devBaseContainerImage := workflowdef.GetDefaultWorkflowDevModeImageTag()
// check if the Platform available
pl, err := platform.GetActivePlatform(ctx, e.C, workflow.Namespace)
if err == nil && len(pl.Spec.DevMode.BaseImage) > 0 {
devBaseContainerImage = pl.Spec.DevMode.BaseImage
}
- propsCM, _, err := e.ensurers.propertiesConfigMap.Ensure(ctx, workflow,
common.WorkflowPropertiesMutateVisitor(ctx, e.StateSupport.Catalog, workflow,
pl))
+ flowDefCM, _, err := e.ensurers.definitionConfigMap.Ensure(ctx,
workflow, pl, ensureWorkflowDefConfigMapMutator(workflow, pl))
+ if err != nil {
+ return ctrl.Result{Requeue: false}, objs, err
+ }
+ objs = append(objs, flowDefCM)
+ userPropsCM, _, err := e.ensurers.userPropsConfigMap.Ensure(ctx,
workflow, pl)
+ if err != nil {
+ return ctrl.Result{Requeue: false}, objs, err
+ }
+ managedPropsCM, _, err := e.ensurers.managedPropsConfigMap.Ensure(ctx,
workflow, pl, common.UserPropertiesMutateVisitor(ctx, e.StateSupport.Catalog,
workflow, pl, userPropsCM.(*corev1.ConfigMap)))
Review Comment:
Does the `managedProps` include user properties too? Is this to calculate
hash as you are describing in the PR? Can't we let the managed CM hold only
manage props?
##########
controllers/profiles/common/ensurer.go:
##########
@@ -34,7 +34,7 @@ var _ ObjectEnsurer = &defaultObjectEnsurer{}
var _ ObjectEnsurer = &noopObjectEnsurer{}
type ObjectEnsurer interface {
- Ensure(ctx context.Context, workflow *operatorapi.SonataFlow, visitors
...MutateVisitor) (client.Object, controllerutil.OperationResult, error)
+ Ensure(ctx context.Context, workflow *operatorapi.SonataFlow, platform
*operatorapi.SonataFlowPlatform, visitors ...MutateVisitor) (client.Object,
controllerutil.OperationResult, error)
Review Comment:
Instead, can't we have something like:
```go
type ObjectEnsurer interface {
Ensure(ctx context.Context, workflow *operatorapi.SonataFlow, visitors
...MutateVisitor) (client.Object, controllerutil.OperationResult, error)
WithPlatform(platform *operatorapi.SonataFlowPlatform)
}
```
Then in the ensurer that requires the platform you do
`ensurer.WithPlatform(p).Ensure(...)`.
##########
controllers/profiles/common/ensurer.go:
##########
@@ -62,10 +62,10 @@ type defaultObjectEnsurer struct {
creator ObjectCreator
}
-func (d *defaultObjectEnsurer) Ensure(ctx context.Context, workflow
*operatorapi.SonataFlow, visitors ...MutateVisitor) (client.Object,
controllerutil.OperationResult, error) {
+func (d *defaultObjectEnsurer) Ensure(ctx context.Context, workflow
*operatorapi.SonataFlow, platform *operatorapi.SonataFlowPlatform, visitors
...MutateVisitor) (client.Object, controllerutil.OperationResult, error) {
result := controllerutil.OperationResultNone
- object, err := d.creator(workflow)
+ object, err := d.creator(workflow, platform)
Review Comment:
Here `defaultObjectEnsurer` can hold the `platform` and do a nil check to
call the creator in the same fashion:
```go
if (d.platform == nil) {
object, err := d.creator(workflow)
} else {
object, err := d.withPlatform(platform).creator(workflow)
}
```
This way I think we can preserve the API since mutators and creators having
a new unused parameter might be hard to maintain as you said in the chat.
##########
workflowproj/workflowproj.go:
##########
@@ -38,6 +38,8 @@ import (
operatorapi
"github.com/apache/incubator-kie-kogito-serverless-operator/api/v1alpha08"
)
+// TODO: should we delete this whole file? seems used only the the associated
test functions
Review Comment:
What do you mean? This is an external module that an actor can use to
generate CRs based on SonataFlow projects. kn workflow uses it, for instance.
But I'm curious to understand why you came to this conclusion. Maybe we need
to communicate this better, as part of being part of the
[README](https://github.com/apache/incubator-kie-kogito-serverless-operator/blob/main/workflowproj/README.md).
--
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]
