ricardozanini commented on code in PR #316:
URL:
https://github.com/apache/incubator-kie-kogito-serverless-operator/pull/316#discussion_r1420377452
##########
bundle/manifests/sonataflow-operator.clusterserviceversion.yaml:
##########
@@ -536,6 +536,42 @@ spec:
- subjectaccessreviews
verbs:
- create
+ - apiGroups:
Review Comment:
Nice catch!
##########
controllers/profiles/common/app_properties.go:
##########
@@ -267,6 +268,35 @@ func generateDiscoveryProperties(ctx context.Context,
catalog discovery.ServiceC
}
}
}
+
+ for _, function := range workflow.Spec.Flow.Functions {
Review Comment:
This method is getting quite large. Maybe a little refactoring here to an
`app_properties_discovery.go` file that would wrap al of this? wdyt?
##########
controllers/discovery/discovery.go:
##########
@@ -84,10 +89,10 @@ type sonataFlowServiceCatalog struct {
}
// NewServiceCatalog returns a new ServiceCatalog configured to resolve
kubernetes, knative, and openshift resource addresses.
-func NewServiceCatalog(cli client.Client) ServiceCatalog {
+func NewServiceCatalog(cli client.Client, knServingClient
clientservingv1.ServingV1Interface, knEventingClient
clienteventingv1.EventingV1Interface) ServiceCatalog {
Review Comment:
We can have a `DiscoveryClient` struct to hold all this and share it here as
a pointer. Wdyt?
##########
controllers/discovery/discovery_test.go:
##########
@@ -116,7 +116,7 @@ func doTesQueryKubernetesDeploymentWithService(t
*testing.T, outputFormat string
service.Spec.Type = corev1.ServiceTypeNodePort
cli := fake.NewClientBuilder().WithRuntimeObjects(deployment,
service).Build()
- ctg := NewServiceCatalog(cli)
+ ctg := NewServiceCatalog(cli, nil, nil)
Review Comment:
Having a `DiscoveryClient` you can get rid of passing `nil` here.
`NewKnativeDiscoveryClient(cli, knativeEven, knativeSvc)` and
`NewDiscoveryClient(cli)`. Alternatively, the `NewServiceCatalog` could create
this knative clients internally if it only depends on `client`.
##########
utils/cluster.go:
##########
@@ -37,6 +41,36 @@ func SetIsOpenShift(cfg *rest.Config) {
var err error
isOpenShift, err = openshift.IsOpenShift(cfg)
if err != nil {
- panic("Impossible to verify if the cluster is OpenShift or not"
+ err.Error())
+ panic("Impossible to verify if the cluster is OpenShift or not:
" + err.Error())
+ }
+}
+
+// IsKnativeServing is a global flag that can be safely called across
reconciliation cycles, defined at the controller manager start.
+func IsKnativeServing() bool {
+ return isKnativeServing
+}
+
+// IsKnativeEventing is a global flag that can be safely called across
reconciliation cycles, defined at the controller manager start.
+func IsKnativeEventing() bool {
+ return isKnativeEventing
+}
+
+// SetIsKnative sets the global flags isKnativeServing and isKnativeEventing
by the controller manager.
+func SetIsKnative(cfg *rest.Config) {
+ if cli, err := discovery.NewDiscoveryClientForConfig(cfg); err != nil {
+ panic("Impossible to verify if the knative system is installed
in the cluster: " + err.Error())
Review Comment:
Like I said in the previous comment, if we can't get a Knative client, we
disable discovery for their objects or any future feature we may have.
##########
utils/knative/common.go:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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 knative
Review Comment:
We can have a topmost `knative` package to implement everything I'm
proposing in the other comments.
##########
main.go:
##########
@@ -85,9 +93,34 @@ func main() {
}
utils.SetIsOpenShift(mgr.GetConfig())
+ utils.SetIsKnative(mgr.GetConfig())
+
+ if utils.IsKnativeServing() {
+ klog.V(log.I).Infof("Knative serving is installed in the
cluster")
+ if knativeServingClient, err =
knative.NewKnativeServingClient(mgr.GetConfig()); err != nil {
+ if err != nil {
+ klog.V(log.E).ErrorS(err, "unable to connect
with the knative serving system")
+ os.Exit(1)
+ }
+ }
+ }
+
+ if utils.IsKnativeEventing() {
+ klog.V(log.I).Infof("Knative eventing is installed in the
cluster")
+ if knativeEventingClient, err =
knative.NewKnativeEventingClient(mgr.GetConfig()); err != nil {
+ if err != nil {
+ klog.V(log.E).ErrorS(err, "unable to connect
with the knative eventing system")
+ os.Exit(1)
+ }
+ }
+ }
Review Comment:
Here's not the best place to add this. If I install this operator first and
then knative, I won't have these clients configured. We have to carry the
`RestConfig` to the controllers and create the Knative clients during the
reconciliation.
Alternatively, we can have a factory for these clients to hold them in a
singleton pattern. In every reconciliation cycle, we can simply check if
Knative is still installed in the cluster and return the client we have.
This accessor then can be used in discovery and we avoid passing all this
construct `ProfileExtensions` around.
Something like `knative.GetClient()` will check if there's an instance
ready, if not we create one, cache, and return.
Also, this is not a required state, meaning we can't `os.Exit(1)` if we
can't create the client.
##########
utils/cluster.go:
##########
@@ -37,6 +41,36 @@ func SetIsOpenShift(cfg *rest.Config) {
var err error
isOpenShift, err = openshift.IsOpenShift(cfg)
if err != nil {
- panic("Impossible to verify if the cluster is OpenShift or not"
+ err.Error())
+ panic("Impossible to verify if the cluster is OpenShift or not:
" + err.Error())
Review Comment:
Here it's fine to have a terminal state since being or not OpenShift is
deterministic.
--
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]
