wmedvede commented on code in PR #372:
URL:
https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/372#discussion_r1475809671
##########
api/v1alpha08/sonataflowplatform_types.go:
##########
@@ -46,6 +46,22 @@ type SonataFlowPlatformSpec struct {
// `sonataflow.org/profile: prod`
// +optional
Services ServicesPlatformSpec `json:"services,omitempty"`
+ // Persistence defines the platform persistence configuration. When
this field is set,
+ // the configuration is used as the persistence for platform services
and sonataflow instances
+ // that don't provide one of their own.
+ // +optional
+ Persistence *PlatformPersistenceSpec `json:"persistence,omitempty"`
Review Comment:
Sorry for being picky, I haven't note this before, not a blocker...
But, basically we have.
BuildPlatformSpec, DevModePlatformSpec, SerivicesPlatormSpec and now we
introduce PlatformPersistenceSpec, shouldn't this be PersistencePlatformSpec
instead?
##########
controllers/profiles/common/object_creators.go:
##########
@@ -157,7 +159,13 @@ func defaultContainer(workflow *operatorapi.SonataFlow)
(*corev1.Container, erro
if err := mergo.Merge(defaultFlowContainer,
workflow.Spec.PodTemplate.Container.ToContainer(), mergo.WithOverride); err !=
nil {
return nil, err
}
- defaultFlowContainer = ConfigurePersistence(defaultFlowContainer,
workflow.Spec.Persistence, defaultSchemaName, workflow.Namespace)
+ if workflow.Spec.Persistence != nil {
Review Comment:
Please correct me if I'm wrong, but believe this code is not considering the
case where the workflow don't have a configured persistence, but the platform
has. In that case, we should try to get it from the platform.
I am correct?
##########
controllers/platform/initialize.go:
##########
@@ -106,6 +107,13 @@ func (action *initializeAction) Handle(ctx
context.Context, platform *operatorap
}
platform.Status.Version = metadata.SpecVersion
+ // store workflow persistence configuration
+ if platform.Spec.Persistence != nil {
+ persistence.WorkflowConfig.SetConfig(platform.Spec.Persistence)
Review Comment:
> I should add a level of abstraction to catch configuration based on the
namespace, not a single one. I'll update that.
Yes, this was my question. The platform is for the namespace, all good, and
yeah, we can have only one platform persistence configuration per
platform/namespace.
But my question was if this caching you are doing is "global" or per
"namespace", that's the part I missing.
##########
controllers/profiles/common/object_creators.go:
##########
@@ -218,10 +226,21 @@ func WorkflowPropsConfigMapCreator(workflow
*operatorapi.SonataFlow) (client.Obj
return workflowproj.CreateNewAppPropsConfigMap(workflow, props), nil
}
-func ConfigurePersistence(serviceContainer *corev1.Container, options
*operatorapi.PersistenceOptions, defaultSchema, namespace string)
*corev1.Container {
+func ConfigurePersistence(serviceContainer *corev1.Container, config
*operatorapi.PersistenceOptions, defaultSchema, namespace string)
(*corev1.Container, error) {
+ if config == nil {
+ return serviceContainer, nil
+ }
c := serviceContainer.DeepCopy()
- if options != nil && options.PostgreSql != nil {
- c.Env = append(c.Env,
persistence.ConfigurePostgreSqlEnv(options.PostgreSql, defaultSchema,
namespace)...)
+
+ var p *operatorapi.PersistencePostgreSQL
+ if config.PostgreSQL == nil &&
persistence.WorkflowConfig.GetPostgreSQLConfiguration() == nil {
+ return nil, fmt.Errorf("no persistence specification available
in the workflow or in the platform")
+ }
+ if config.PostgreSQL != nil {
+ p = config.PostgreSQL
Review Comment:
If the workflow persistence was set, we need to give the chance to see if a
schema was provided, and if it's provided, below we must use that schema
instead of the defaultSchema that we receive as parameter.
In this way we give users the chance to fine tune the schema name for a
particular workflow.
##########
controllers/profiles/common/object_creators.go:
##########
@@ -218,10 +226,21 @@ func WorkflowPropsConfigMapCreator(workflow
*operatorapi.SonataFlow) (client.Obj
return workflowproj.CreateNewAppPropsConfigMap(workflow, props), nil
}
-func ConfigurePersistence(serviceContainer *corev1.Container, options
*operatorapi.PersistenceOptions, defaultSchema, namespace string)
*corev1.Container {
+func ConfigurePersistence(serviceContainer *corev1.Container, config
*operatorapi.PersistenceOptions, defaultSchema, namespace string)
(*corev1.Container, error) {
+ if config == nil {
+ return serviceContainer, nil
+ }
c := serviceContainer.DeepCopy()
- if options != nil && options.PostgreSql != nil {
- c.Env = append(c.Env,
persistence.ConfigurePostgreSqlEnv(options.PostgreSql, defaultSchema,
namespace)...)
+
+ var p *operatorapi.PersistencePostgreSQL
+ if config.PostgreSQL == nil &&
persistence.WorkflowConfig.GetPostgreSQLConfiguration() == nil {
+ return nil, fmt.Errorf("no persistence specification available
in the workflow or in the platform")
+ }
+ if config.PostgreSQL != nil {
+ p = config.PostgreSQL
+ } else if persistence.WorkflowConfig.GetPostgreSQLConfiguration() !=
nil {
+ p = persistence.WorkflowConfig.GetPostgreSQLConfiguration()
Review Comment:
if we pick the platform configuration, yes, we must use the defaultSchema as
it comes, basically the workflow name.
--
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]
