wmedvede commented on code in PR #367:
URL:
https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/367#discussion_r1476013371
##########
controllers/profiles/common/ensurer.go:
##########
@@ -36,6 +36,9 @@ var _ ObjectEnsurer = &noopObjectEnsurer{}
type ObjectEnsurer interface {
Ensure(ctx context.Context, workflow *operatorapi.SonataFlow, visitors
...MutateVisitor) (client.Object, controllerutil.OperationResult, error)
}
+type ObjectEnsurerWithPlatform interface {
Review Comment:
Here I'm just thinking aloud, not requesting a change.
But, what I see is that the Ensure method right now applies always with a
provided workflow, the both existing ObjectEnsurer and the new
ObjectEnsurerWithPlatform., all good.
Then, by definition, to deploy/and create the workflows we need always a
platform, the so called active platform.
I'm wondering if we really need to maintain this two interfaces, or simply
add the platform as parameter to the already existing ObjectEnsurer.
I am correct?
@ricardozanini @dmartinol
##########
controllers/profiles/common/ensurer.go:
##########
@@ -84,6 +95,34 @@ func (d *defaultObjectEnsurer) Ensure(ctx context.Context,
workflow *operatorapi
return object, result, nil
}
+// defaultObjectEnsurerWithPlatform is the equivalent of defaultObjectEnsurer
for resources that require a reference to the SonataFlowPlatform
+type defaultObjectEnsurerWithPlatform struct {
Review Comment:
nitpick...., maybe we can add a declaration like this
var _ ObjectEnsurerWithPlatform = &defaultObjectEnsurerWithPlatform{} as
it's being done with the other ensurers to let the compiler ensure the
interface is properly implemented.
##########
controllers/profiles/common/properties/application.go:
##########
@@ -75,36 +75,34 @@ func (a *appPropertyHandler) WithServiceDiscovery(ctx
context.Context, catalog d
}
func (a *appPropertyHandler) Build() string {
- var props *properties.Properties
+ var userProps *properties.Properties
Review Comment:
Related to the comment I did before, this ApplicationPropertyHandler has
changed the original semantic. Maybe we should rename and update a bit the
documentation of the function NewAppPropertyHandler.
##########
controllers/profiles/common/mutate_visitors.go:
##########
@@ -105,32 +105,28 @@ func ServiceMutateVisitor(workflow
*operatorapi.SonataFlow) MutateVisitor {
}
}
-func WorkflowPropertiesMutateVisitor(ctx context.Context, catalog
discovery.ServiceCatalog,
- workflow *operatorapi.SonataFlow, platform
*operatorapi.SonataFlowPlatform) MutateVisitor {
+func UserPropertiesMutateVisitor(ctx context.Context, catalog
discovery.ServiceCatalog,
Review Comment:
Maybe we should name this ManagedPropertiesMutateVisitor instead, wdyt?
##########
controllers/profiles/common/test/common.go:
##########
@@ -0,0 +1,84 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you 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_test
+
+import (
+ "context"
+
+
"github.com/apache/incubator-kie-kogito-serverless-operator/controllers/discovery"
+)
+
+const (
Review Comment:
This file was originally generated to leave in the context of the properties
tests package, why do we need to move it outside it?
The question is because as we move to a common package the trend is that
it'll degenerate and become a sort of all in one where people start putting the
stuff their need for whatever test, etc.
##########
controllers/profiles/common/mutate_visitors.go:
##########
@@ -105,32 +105,28 @@ func ServiceMutateVisitor(workflow
*operatorapi.SonataFlow) MutateVisitor {
}
}
-func WorkflowPropertiesMutateVisitor(ctx context.Context, catalog
discovery.ServiceCatalog,
- workflow *operatorapi.SonataFlow, platform
*operatorapi.SonataFlowPlatform) MutateVisitor {
+func UserPropertiesMutateVisitor(ctx context.Context, catalog
discovery.ServiceCatalog,
+ workflow *operatorapi.SonataFlow, platform
*operatorapi.SonataFlowPlatform, userProps *corev1.ConfigMap) MutateVisitor {
return func(object client.Object) controllerutil.MutateFn {
return func() error {
- if kubeutil.IsObjectNew(object) {
- return nil
- }
- cm := object.(*corev1.ConfigMap)
- cm.Labels = workflow.GetLabels()
- _, hasKey :=
cm.Data[workflowproj.ApplicationPropertiesFileName]
+ managedProps := object.(*corev1.ConfigMap)
+ managedProps.Labels = workflow.GetLabels()
+ _, hasKey :=
managedProps.Data[workflowproj.GetManagedPropertiesFileName(workflow)]
if !hasKey {
- cm.Data = make(map[string]string, 1)
- props, err :=
properties.ImmutableApplicationProperties(workflow, platform)
- if err != nil {
- return err
- }
-
cm.Data[workflowproj.ApplicationPropertiesFileName] = props
- return nil
+ managedProps.Data = make(map[string]string, 1)
+
managedProps.Data[workflowproj.GetManagedPropertiesFileName(workflow)] = ""
}
+ userProperties, hasKey :=
userProps.Data[workflowproj.ApplicationPropertiesFileName]
+ if !hasKey {
+ userProperties = ""
+ }
// In the future, if this needs change, instead we can
receive an AppPropertyHandler in this mutator
props, err :=
properties.NewAppPropertyHandler(workflow, platform)
if err != nil {
return err
}
- cm.Data[workflowproj.ApplicationPropertiesFileName] =
props.WithUserProperties(cm.Data[workflowproj.ApplicationPropertiesFileName]).
+
managedProps.Data[workflowproj.GetManagedPropertiesFileName(workflow)] =
props.WithUserProperties(userProperties).
Review Comment:
I think this code for people that comes from the previous version is a bit
confusing in the first read, because before, the user properties where combined
with the properties we where generating. And now, with the internal refactoring
you did, the user properties are being used as a kind of "candidates for doing
the service discovery", but won't be included in the produced properties set.
Maybe, now that we are going to keep to properties living in parallel, the
original application.properties, plus the new application-pod.properties
generated by this handler, we can rename to something like
ManagedAppPropertiesHandler (or a better name) to distinguish the new
situation.
Just thinking aloud.
--
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]
