squakez commented on code in PR #6902: URL: https://github.com/apache/camel-k/pull/6902#discussion_r4456433359
########## .github/workflows/camel-monitor.yml: ########## @@ -0,0 +1,97 @@ +# --------------------------------------------------------------------------- +# 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. +# --------------------------------------------------------------------------- + +name: camel-monitor + +env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + CAMEL_K_TEST_TIMEOUT_SHORT: 2m + CAMEL_K_TEST_TIMEOUT_MEDIUM: 5m + CAMEL_K_TEST_TIMEOUT_LONG: 10m + +on: + pull_request: + branches: + - main + - "release-*" + paths-ignore: + - 'docs/**' + - 'java/**' + - 'proposals/**' + - '**.adoc' + - '**.md' + - 'KEYS' + - 'LICENSE' + - 'NOTICE' + push: + branches: + - main + - "release-*" + paths-ignore: + - 'docs/**' + - 'java/**' + - 'proposals/**' + - '**.adoc' + - '**.md' + - 'KEYS' + - 'LICENSE' + - 'NOTICE' + workflow_dispatch: + +concurrency: + group: ${{ github.workflow }}-${{ github.event.pull_request.number || github.sha }} + cancel-in-progress: true + +jobs: + camel-monitor: + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v7 + with: + persist-credentials: false + submodules: recursive + + - name: Infra setting + uses: ./.github/actions/infra-setting + + - name: Install Camel Monitor operator + shell: bash + run: | + go mod download github.com/camel-tooling/camel-monitor-operator Review Comment: Quite complicated procedure... just run `kubectl apply -k github.com/camel-tooling/camel-monitor-operator/install/overlays/all-namespaces?ref=v1.0.0 --server-side` which installs by default in `camel` namespace. ########## pkg/controller/integration/integration_controller.go: ########## @@ -64,10 +66,20 @@ func Add(ctx context.Context, mgr manager.Manager, c client.Client) error { return fmt.Errorf("unable to set up field indexer for status.phase: %w", err) } - return add(mgr, c, newReconciler(mgr, c)) + r, err := newReconciler(mgr, c) + if err != nil { + return err + } + + return add(mgr, c, r) } -func newReconciler(mgr manager.Manager, c client.Client) reconcile.Reconciler { +func newReconciler(mgr manager.Manager, c client.Client) (reconcile.Reconciler, error) { Review Comment: Why this change? I don't see the reason to do any change here either. We can keep the existing code as it was. ########## pkg/controller/integration/integration_controller.go: ########## @@ -64,10 +66,20 @@ func Add(ctx context.Context, mgr manager.Manager, c client.Client) error { return fmt.Errorf("unable to set up field indexer for status.phase: %w", err) } - return add(mgr, c, newReconciler(mgr, c)) + r, err := newReconciler(mgr, c) Review Comment: The previous code was doing the same, no need to change. ########## pkg/controller/integration/monitor.go: ########## @@ -46,12 +48,19 @@ import ( ) // NewMonitorAction is an action used to monitor manager Integrations. -func NewMonitorAction() Action { - return &monitorAction{} +func NewMonitorAction(c client.Client) (Action, error) { Review Comment: Proabably the api check should be performed by the caller instead, so we can manage there the error and don't change this func signature. We can expect to pass here the `camelMonitorAvailable` flag instead. ########## script/Makefile: ########## @@ -338,6 +338,13 @@ test-knative: gotestfmt-install go test -timeout 60m -v ./e2e/knative -tags=integration $(GOTESTFMT) || ((FAILED++)); \ exit $${FAILED} +# +# CamelMonitor discovery tests require Camel Monitor and Camel K operators. +# +.PHONY: test-camel-monitor +test-camel-monitor: gotestfmt-install + set -o pipefail; go test -p 1 -timeout 20m -v ./e2e/camelmonitor --parallel 1 -tags=integration $(GOTESTFMT) Review Comment: ```suggestion go test -timeout 20m -v ./e2e/camelmonitor -tags=integration $(GOTESTFMT) || ((FAILED++)); \ ``` ########## pkg/controller/integration/integration_controller.go: ########## @@ -428,6 +446,24 @@ func watchKnativeResources(c client.Client, b *builder.Builder) error { return nil } +func watchCamelMonitorResources(c client.Client, b *builder.Builder) error { + installed, err := kubernetes.IsAPIResourceInstalled(c, monitorv1alpha1.SchemeGroupVersion.String(), monitorv1alpha1.AppKind) + if err != nil { + return err + } + if !installed { + log.Info("CamelMonitor resources are not installed in the cluster. To enable monitoring discovery after installing the CRD, " + + "assign the required RBAC privileges and restart the Camel K operator.") + + return nil + } + log.Info("CamelMonitor resources installed in the cluster: make sure to assign CamelMonitor RBAC privileges") + b.Watches(&monitorv1alpha1.CamelMonitor{}, &handler.EnqueueRequestForObject{}, + builder.WithPredicates(predicate.Funcs{UpdateFunc: func(event.UpdateEvent) bool { return false }})) Review Comment: Probably it makes sense `builder.WithPredicates(StatusChangedPredicate{})`, wdyt? -- 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]
