Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughPrometheus provisioning was deprecated: the operator no longer creates Prometheus CRs, Routes, or Ingresses and will delete existing ones. Enabling prometheus now only creates ServiceMonitors and PrometheusRules for metrics scraping. CRDs, CSVs, controllers, docs, and tests updated to reflect this behavior. Changes
Sequence Diagram(s)mermaid ArgoCD_CR->>Operator: Reconcile request Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Rizwana777
force-pushed
the
issue-8738-deprecate-prometheus-cr
branch
from
2b6cc44 to
03cc585
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
bundle/manifests/argocd-operator.clusterserviceversion.yaml (2)
655-686:⚠️ Potential issue | 🟡 MinorPrometheus ingress/route descriptors still imply creation.
The new enabled description states Route/Ingress are deprecated and no longer created, but the
prometheus.ingress.enabledandprometheus.route.enableddescriptors still claim they toggle creation. Please mark these as deprecated/ignored to avoid user confusion.🛠️ Suggested descriptor updates
- - description: Enabled will toggle the creation of the Ingress. + - description: 'Deprecated: This field is no longer used and will be ignored.' displayName: Ingress Enabled path: prometheus.ingress.enabled @@ - - description: Enabled will toggle the creation of the OpenShift Route. + - description: 'Deprecated: This field is no longer used and will be ignored.' displayName: Route Enabled path: prometheus.route.enabled
1256-1287:⚠️ Potential issue | 🟡 MinorSame deprecation mismatch in v1beta1 descriptors.
In the v1beta1 CRD section, ingress/route descriptors still say they toggle creation, which conflicts with the new deprecation statement. Please align those descriptions as deprecated/ignored.
🛠️ Suggested descriptor updates
- - description: Enabled will toggle the creation of the Ingress. + - description: 'Deprecated: This field is no longer used and will be ignored.' displayName: Ingress Enabled path: prometheus.ingress.enabled @@ - - description: Enabled will toggle the creation of the OpenShift Route. + - description: 'Deprecated: This field is no longer used and will be ignored.' displayName: Route Enabled path: prometheus.route.enabledconfig/manifests/bases/argocd-operator.clusterserviceversion.yaml (2)
477-492:⚠️ Potential issue | 🟡 MinorInconsistent deprecation documentation for
prometheus.ingress.enabledandprometheus.route.enabled.The descriptions for
prometheus.ingress.enabled(line 477) andprometheus.route.enabled(line 485) still state they will "toggle the creation" of the respective resources. However, per this PR, Prometheus Ingress and Route are deprecated and will no longer be created. These descriptions should be updated to indicate deprecation similar to howprometheus.hostandprometheus.sizewere updated.Suggested description updates
For
prometheus.ingress.enabled:- description: 'Deprecated: Prometheus Ingress is no longer created by the operator. This field will be ignored.'For
prometheus.route.enabled:- description: 'Deprecated: Prometheus Route is no longer created by the operator. This field will be ignored.'
1101-1116:⚠️ Potential issue | 🟡 MinorSame inconsistency exists in the v1alpha1 section.
The
prometheus.ingress.enabledandprometheus.route.enableddescriptions in the v1alpha1 ArgoCD spec (lines 1101-1116) have the same issue - they should also be marked as deprecated.docs/usage/insights.md (1)
137-145:⚠️ Potential issue | 🟡 MinorRemove the top-level
routeconfiguration from the example.The ArgoCD CRD spec does not support a top-level
routefield. Theroutefield only exists under component-specific specs (e.g.,server.route,prometheus.route). The top-levelroute: enabled: trueshould be removed from the YAML example. Theserver.route: enabled: trueis valid and should remain.Note: The
prometheus.routefield is deprecated and ignored by the operator, so theprometheus.enabled: trueline alone is sufficient.
🤖 Fix all issues with AI agents
In `@docs/reference/argocd.md`:
- Line 787: Update the docs to remove the contradiction: change the short
description for the top-level `enabled` field so it accurately states that
ServiceMonitors are created when `enabled: true` but PrometheusRules are only
created when both `enabled: true` and `.spec.monitoring.enabled: true`;
alternatively, add a brief documented subsection for `.spec.monitoring.enabled`
explaining its boolean role and the conditional creation of PrometheusRules
(reference `.spec.monitoring.enabled`, `enabled`, ServiceMonitors,
PrometheusRules in the text).
In `@tests/ginkgo/parallel/1-123_validate_prometheus_cr_deleted_test.go`:
- Line 183: The test's description string in the It block "verifies Prometheus
Service, Route, and Ingress are deleted if they exist" is inaccurate because the
code only asserts Service and Ingress; either add explicit Route verification or
update the description. To fix: either (a) add a conditional OpenShift Route
check after the Ingress verification that queries the Route resource (using the
cluster's Route client/routev1 API and the same namespace/name used elsewhere)
and asserts the Route is absent, or (b) change the It(...) description to remove
"Route" so it only mentions Service and Ingress; locate the test by the It
description in the function containing the verification blocks and update
accordingly.
🧹 Nitpick comments (4)
tests/ginkgo/parallel/1-123_validate_prometheus_cr_deleted_test.go (2)
224-233: Clarify the Service existence check logic.The test declares a
prometheusServicebut only performs aGetoperation to check if one already exists (from a previous installation). The comment on line 227 clarifies the intent, but the surrounding "manually creating" By statement (line 201) is misleading since the code doesn't actually create the service - it just checks for its existence.Consider updating the
Bystatement to better reflect the actual behavior:Suggested clarification
- By("manually creating a Prometheus Service to simulate existing installation") + By("checking if a Prometheus Service exists from a previous installation")
48-52: Consider extractingk8sClientinitialization error handling.The error return from
GetE2ETestKubeClient()is being discarded (line 50). While this may be intentional if errors cause test framework failures, explicitly handling or asserting on the error would be more robust.Suggested improvement
BeforeEach(func() { fixture.EnsureParallelCleanSlate() - k8sClient, _ = fixtureUtils.GetE2ETestKubeClient() + var err error + k8sClient, err = fixtureUtils.GetE2ETestKubeClient() + Expect(err).ToNot(HaveOccurred()) ctx = context.Background() })controllers/argocd/prometheus.go (1)
34-76: Remove unused helper functions.With Prometheus CR creation/update removed, the following functions appear to be dead code:
getPrometheusReplicas(lines 43-52)hasPrometheusSpecChanged(lines 59-76)Note:
getPrometheusHostis still in use bycontrollers/argocd/secret.goand should be retained.docs/reference/argocd.md (1)
817-817: Incomplete explanation of created resources.Line 817 states "This will create ServiceMonitor resources" but according to the table at line 787 and the Important Changes note, both ServiceMonitors and PrometheusRules may be created. The explanation should mention both resource types for completeness.
Proposed clarification
-This will create ServiceMonitor resources that allow your existing Prometheus instance to discover and scrape metrics from Argo CD components. +This will create ServiceMonitor resources that allow your existing Prometheus instance to discover and scrape metrics from Argo CD components. PrometheusRules for alerting will also be created if monitoring is enabled.
tests/ginkgo/parallel/1-123_validate_prometheus_cr_deleted_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/argocd/prometheus.go (1)
43-52:⚠️ Potential issue | 🟠 MajorRemove unused
getPrometheusReplicasto fix lint failure.The function is unused after the Prometheus CR deprecation changes. No references exist in the codebase—only the definition at line 44. Remove it to resolve the CI lint failure.
Suggested fix
-// getPrometheusSize will return the size value for the Prometheus replica count. -func getPrometheusReplicas(cr *argoproj.ArgoCD) *int32 { - replicas := common.ArgoCDDefaultPrometheusReplicas - if cr.Spec.Prometheus.Size != nil { - if *cr.Spec.Prometheus.Size >= 0 && *cr.Spec.Prometheus.Size != replicas { - replicas = *cr.Spec.Prometheus.Size - } - } - return &replicas -}
🤖 Fix all issues with AI agents
In `@docs/reference/argocd.md`:
- Around line 793-799: The admonition starting with the "!!! note \"Important
Changes\"" block triggers markdownlint MD046 due to an indented code-block
style; fix it by either disabling MD046 for that block or reformatting the
content so it no longer uses the indented code style—e.g., add a
markdownlint-disable comment before the block (disable MD046) or convert the
indented list/paragraphs inside the note to standard fenced formatting; ensure
references like `.spec.prometheus.enabled` and `.spec.monitoring.enabled` remain
intact and correctly formatted.
In `@docs/usage/insights.md`:
- Around line 137-139: The top-level spec.route block is invalid for the ArgoCD
CR; locate the YAML that contains the route: enabled: true entry (the top-level
"route" under "spec") and either remove that block or move it under spec.server
as spec.server.route so it becomes spec.server.route: enabled: true; ensure any
component-specific route entries follow the CRD pattern (e.g.,
spec.server.route) and remove the unsupported top-level spec.route.
In `@tests/ginkgo/parallel/1-123_validate_prometheus_cr_deleted_test.go`:
- Around line 201-233: The test currently only checks for existence with
k8sClient.Get and skips creation, so create the Prometheus Service before
asserting deletion: after constructing prometheusService, attempt to
k8sClient.Create(prometheusService) and ignore an AlreadyExists error (use
errors.IsAlreadyExists(err)), otherwise fail on other errors; then keep the
Existing Expect(err).To(BeNil()) path and use Eventually(prometheusService,
"2m", "5s").Should(k8sFixture.NotExistByName()) to assert it gets deleted.
Ensure you reference prometheusService, k8sClient.Create, errors.IsAlreadyExists
and Eventually(...) in the change.
tests/ginkgo/parallel/1-123_validate_prometheus_cr_deleted_test.go
Outdated
Show resolved
Hide resolved
Rizwana777
force-pushed
the
issue-8738-deprecate-prometheus-cr
branch
from
03cc585 to
26e4025
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bundle/manifests/argocd-operator.clusterserviceversion.yaml (1)
655-688:⚠️ Potential issue | 🟡 MinorClarify when PrometheusRules are created.
The descriptor says PrometheusRules are created when
prometheus.enabledis true, but the controller gates PrometheusRules onspec.monitoring.enabled. Please align the text to avoid misleading users.💬 Suggested wording update
- - description: Enabled will toggle Prometheus support globally for ArgoCD. When - set to true, ServiceMonitors and PrometheusRules will be created for Argo - CD metrics. The Prometheus CR, Route, and Ingress are deprecated and will - no longer be created. + - description: Enabled will toggle Prometheus support globally for ArgoCD. When + set to true, ServiceMonitors will be created for Argo CD metrics. PrometheusRules + for workload status alerts are created when spec.monitoring.enabled is true. + The Prometheus CR, Route, and Ingress are deprecated and will no longer be created.Also applies to: 1256-1289
🧹 Nitpick comments (1)
deploy/olm-catalog/argocd-operator/0.18.0/argocd-operator.v0.18.0.clusterserviceversion.yaml (1)
671-686: Consider adding deprecation notices toprometheus.ingress.enabledandprometheus.route.enableddescriptions.The descriptions for
prometheus.ingress.enabled(lines 671-678) andprometheus.route.enabled(lines 679-686) still state "Enabled will toggle the creation of the Ingress/Route" but these fields are now deprecated and ignored. Consider updating these descriptions to match the deprecation pattern used forhostandsize.
Rizwana777
force-pushed
the
issue-8738-deprecate-prometheus-cr
branch
from
26e4025 to
63c8a97
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v1alpha1/argocd_conversion_test.go (1)
38-47:⚠️ Potential issue | 🔴 CriticalFix invalid composite literal (
&v1beta1{}won’t compile).
v1beta1is a package alias, not a type. This breaks the test build. Use the concrete typev1beta1.ArgoCD.🛠️ Proposed fix
-func makeTestArgoCDBeta(opts ...argoCDBetaOpt) *v1beta1.ArgoCD { - a := &v1beta1{ +func makeTestArgoCDBeta(opts ...argoCDBetaOpt) *v1beta1.ArgoCD { + a := &v1beta1.ArgoCD{ ObjectMeta: metav1.ObjectMeta{ Name: "test-argocd", Namespace: "default", Labels: map[string]string{ "example": "conversion", }, }, }
🤖 Fix all issues with AI agents
In `@config/manifests/bases/argocd-operator.clusterserviceversion.yaml`:
- Around line 461-471: Update the two descriptor description strings so they no
longer state PrometheusRules are created whenever prometheus.enabled is true;
instead clarify that ServiceMonitors are created when prometheus.enabled is true
but PrometheusRules (and other monitoring resources) are only created when
.spec.monitoring.enabled is set; modify the description for the descriptor
referencing prometheus.enabled (and the duplicate block at lines ~1085-1095) to
mention both flags and the exact gating (.spec.monitoring.enabled) so the UI
text is not misleading.
🧹 Nitpick comments (2)
deploy/olm-catalog/argocd-operator/0.18.0/argoproj.io_argocds.yaml (1)
2223-2238: Mark deprecated fields explicitly in the schema.Kubernetes tooling can surface deprecation warnings when
deprecated: trueis set on the field, which complements the description text. Consider adding this for all deprecated Prometheus fields (host/ingress/route/size) in both schema blocks.♻️ Suggested schema tweak
host: description: |- Host is the hostname to use for Ingress/Route resources. Deprecated: This field is no longer used and will be ignored. + deprecated: true type: string ingress: description: |- Ingress defines the desired state for an Ingress for the Prometheus component. Deprecated: This field is no longer used and will be ignored. + deprecated: true properties:controllers/argocd/prometheus.go (1)
136-151: Make delete idempotent by ignoring NotFound.Between
IsObjectFoundandDelete, the CR can disappear; ignoring NotFound avoids transient reconcile errors. Please confirm your controller-runtime version exposesclient.IgnoreNotFound.♻️ Suggested tweak
import ( "context" "fmt" monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/utils/ptr" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/client" @@ if prExists { // Prometheus CR is deprecated, delete it if it exists argoutil.LogResourceDeletion(log, prometheus, "prometheus CR is deprecated and no longer supported") - return r.Delete(context.TODO(), prometheus) + return client.IgnoreNotFound(r.Delete(context.TODO(), prometheus)) }
Rizwana777
force-pushed
the
issue-8738-deprecate-prometheus-cr
branch
2 times, most recently
from
409de82 to
99c3fe9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@docs/usage/insights.md`:
- Around line 3-7: Update the usage/insights.md text to clarify that while the
operator exposes metrics and creates ServiceMonitors, PrometheusRules are only
created when the Argo CD CR has monitoring enabled (i.e.,
.spec.monitoring.enabled == true); reference the controller behavior in
controllers/argocd/prometheus.go that returns early when monitoring is not
enabled and adjust both the Overview paragraph and the earlier mention (near
line ~65) so the docs no longer imply PrometheusRules are created
unconditionally.
In `@tests/ginkgo/parallel/1-123_validate_prometheus_cr_deleted_test.go`:
- Around line 202-273: The test creates unowned resources
(prometheusService/prometheusRoute/prometheusIngress) so the operator's Owns()
watches won't reconcile them; after creating each manual resource, bump a
reconciliation-trigger annotation on the ArgoCD instance used in the test (e.g.,
modify metadata.annotations["reconcile-timestamp"] or similar) by
patching/updating the ArgoCD CR via k8sClient (the same client used for
k8sClient.Create) so the controller will requeue/reconcile and detect/delete the
unowned resources; ensure the patch targets the ArgoCD object variable used in
the test and perform the annotation update immediately after each Create (or
once after all Creates) before asserting NotExistByName with k8sFixture.
🧹 Nitpick comments (1)
config/crd/bases/argoproj.io_argocds.yaml (1)
2213-2226: Make deprecation machine‑readable (not just in descriptions).Descriptions are clear, but tooling won’t emit warnings unless the schema marks fields deprecated. If your target Kubernetes/OpenAPI version supports it, add
deprecated: true(or the supported extension) to deprecated fields (host/ingress and the other deprecated Prometheus fields). Please verify the correct key for your CRD version.Proposed diff (apply similarly to other deprecated fields)
host: description: |- Host is the hostname to use for Ingress/Route resources. Deprecated: This field is no longer used and will be ignored. type: string + deprecated: true ingress: description: |- Ingress defines the desired state for an Ingress for the Prometheus component. Deprecated: This field is no longer used and will be ignored. + deprecated: true properties: annotations:
tests/ginkgo/parallel/1-123_validate_prometheus_cr_deleted_test.go
Outdated
Show resolved
Hide resolved
Rizwana777
force-pushed
the
issue-8738-deprecate-prometheus-cr
branch
from
99c3fe9 to
47c3257
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
bundle/manifests/argocd-operator.clusterserviceversion.yaml (1)
655-689:⚠️ Potential issue | 🟡 MinorAlign Prometheus ingress/route descriptors with deprecation behavior.
The UI descriptions forprometheus.ingress.enabledandprometheus.route.enabledstill say they create resources, but those resources are deprecated and no longer created. Updating these descriptions will avoid misleading users.📝 Suggested doc updates
- - description: Enabled will toggle the creation of the Ingress. + - description: 'Deprecated: Prometheus Ingress is no longer created; this field is ignored.' displayName: Ingress Enabled path: prometheus.ingress.enabled @@ - - description: Enabled will toggle the creation of the OpenShift Route. + - description: 'Deprecated: Prometheus Route is no longer created; this field is ignored.' displayName: Route Enabled path: prometheus.route.enabled @@ - - description: Enabled will toggle the creation of the Ingress. + - description: 'Deprecated: Prometheus Ingress is no longer created; this field is ignored.' displayName: Ingress Enabled path: prometheus.ingress.enabled @@ - - description: Enabled will toggle the creation of the OpenShift Route. + - description: 'Deprecated: Prometheus Route is no longer created; this field is ignored.' displayName: Route Enabled path: prometheus.route.enabledAlso applies to: 1256-1290
deploy/olm-catalog/argocd-operator/0.18.0/argocd-operator.v0.18.0.clusterserviceversion.yaml (1)
655-679:⚠️ Potential issue | 🟡 MinorPrometheus descriptors still conflict with the new deprecation behavior.
Line 655/1256 says PrometheusRules are created whenever
prometheus.enabledis true, but the docs note they’re conditional on.spec.monitoring.enabled. Also, Lines 671–679 and 1273–1287 still describe Ingress/Route creation even though those fields are now deprecated/ignored. Please align the descriptor text to avoid misleading OLM UI hints.🔧 Suggested wording (apply to both v1alpha1 and v1beta1 blocks)
- - description: Enabled will toggle Prometheus support globally for ArgoCD. When - set to true, ServiceMonitors and PrometheusRules will be created for Argo - CD metrics. The Prometheus CR, Route, and Ingress are deprecated and will - no longer be created. + - description: Enabled will toggle Prometheus support globally for ArgoCD. When + set to true, ServiceMonitors will be created for Argo CD metrics. PrometheusRules + are created only when `.spec.monitoring.enabled` is also true. The Prometheus + CR, Route, and Ingress are deprecated and will no longer be created. @@ - - description: Enabled will toggle the creation of the Ingress. + - description: 'Deprecated: This field is no longer used and will be ignored.' @@ - - description: Enabled will toggle the creation of the OpenShift Route. + - description: 'Deprecated: This field is no longer used and will be ignored.'Also applies to: 1256-1289
config/manifests/bases/argocd-operator.clusterserviceversion.yaml (1)
477-492:⚠️ Potential issue | 🟡 MinorConsider adding deprecation notices to
prometheus.ingress.enabledandprometheus.route.enableddescriptors.The
prometheus.enableddescription states that "Route, and Ingress are deprecated and will no longer be created," but these sub-field descriptors still describe themselves as toggling creation without any deprecation notice. This inconsistency could confuse users viewing the CRD in the OLM UI.📝 Suggested wording
- - description: Enabled will toggle the creation of the Ingress. + - description: 'Deprecated: This field is no longer used. The Prometheus Ingress + will not be created.' displayName: Ingress Enabled path: prometheus.ingress.enabled x-descriptors: - urn:alm:descriptor:com.tectonic.ui:fieldGroup:Grafana - urn:alm:descriptor:com.tectonic.ui:fieldGroup:Prometheus - urn:alm:descriptor:com.tectonic.ui:fieldGroup:Server - urn:alm:descriptor:com.tectonic.ui:booleanSwitch - - description: Enabled will toggle the creation of the OpenShift Route. + - description: 'Deprecated: This field is no longer used. The Prometheus Route + will not be created.' displayName: Route Enabled path: prometheus.route.enabled
🧹 Nitpick comments (1)
config/crd/bases/argoproj.io_argocds.yaml (1)
2213-2216: Consider adding CEL validation warnings for deprecated fields.While the description-based deprecation is sufficient, you could optionally add
x-kubernetes-validationsrules to emit warnings at admission time when users configure the deprecated fields. This provides immediate feedback duringkubectl apply.Example for the
hostfield:host: description: |- Host is the hostname to use for Ingress/Route resources. Deprecated: This field is no longer used and will be ignored. type: string x-kubernetes-validations: - rule: "self == ''" messageExpression: "'prometheus.host is deprecated and will be ignored'" reason: FieldValueDeprecatedThis is optional and can be deferred if you prefer to keep the change minimal.
Also applies to: 18302-18305
Rizwana777
force-pushed
the
issue-8738-deprecate-prometheus-cr
branch
3 times, most recently
from
18a2194 to
d19830b
Compare
Rizwana777
force-pushed
the
issue-8738-deprecate-prometheus-cr
branch
3 times, most recently
from
4c6e28c to
3b8f038
Compare
Signed-off-by: Rizwana777 <rizwananaaz177@gmail.com>
Rizwana777
force-pushed
the
issue-8738-deprecate-prometheus-cr
branch
from
3b8f038 to
6337293
Compare
There was a problem hiding this comment.
LGTM, thanks @Rizwana777! I pushed a tiny fix to remove a potential E2E race condition, just waiting for green CI.
Signed-off-by: Jonathan West <jgwest@gmail.com>
jgwest
force-pushed
the
issue-8738-deprecate-prometheus-cr
branch
from
e083457 to
c6a290c
Compare
3 participants
JIRA - https://issues.redhat.com/browse/GITOPS-8738
Summary by CodeRabbit
Deprecations
New Behavior
Documentation
Tests
Chores