cluster/charts/crossplane/templates/podmonitor.yaml (1)

45-45: ⚡ Quick win

honorLabels: true is hardcoded — consider exposing it in values.yaml.

HonorLabels chooses the metric's labels on collisions with target labels. Hardcoding true means users who want label collision protection (e.g., preventing a pod from overriding Prometheus-added job or instance labels) have no way to opt out without forking the chart. This is inconsistent with the rest of the PodMonitor values block, where every other field is configurable.

💡 Suggested fix

Add to values.yaml (under metrics.podMonitor):

     metricRelabelings: []
+    # -- If true, conflicting labels from the target pod take precedence over Prometheus-added labels.
+    honorLabels: true

Then replace the hardcoded field in both podmonitor.yaml and rbac-manager-podmonitor.yaml:

-      honorLabels: true
+      honorLabels: {{ .Values.metrics.podMonitor.honorLabels }}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cluster/charts/crossplane/templates/podmonitor.yaml` at line 45, Expose the
hardcoded honorLabels by adding a values key (e.g.,
metrics.podMonitor.honorLabels, defaulting to true) in values.yaml and then
replace the literal "honorLabels: true" in the PodMonitor templates (look for
honorLabels in podmonitor.yaml and rbac-manager-podmonitor.yaml) with the
templated value (Helm lookup of .Values.metrics.podMonitor.honorLabels, ensuring
proper boolean rendering and a sensible default). Ensure both templates use the
same .Values key so users can opt in/out consistently.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

cluster/charts/crossplane/templates/rbac-manager-podmonitor.yaml (1)

8-10: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Metadata is still missing release: {{ .Release.Name }} (previously flagged).

The sibling podmonitor.yaml includes release: {{ .Release.Name }} in metadata labels, but this file omits it. Any podMonitorSelector that relies on the release label would discover the Crossplane PodMonitor but miss this one.

🛠️ Proposed fix

  labels:
    app: {{ template "crossplane.name" . }}
+   release: {{ .Release.Name }}
    {{- include "crossplane.labels" . | indent 4 }}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cluster/charts/crossplane/templates/rbac-manager-podmonitor.yaml` around
lines 8 - 10, The metadata labels block in rbac-manager-podmonitor.yaml is
missing the release label, so add release: {{ .Release.Name }} to the metadata
labels alongside app: {{ template "crossplane.name" . }} and the included {{-
include "crossplane.labels" . | indent 4 }} so this PodMonitor can be selected
by any podMonitorSelector that filters on release (mirror the sibling
podmonitor.yaml's labels); update the labels section in the
rbac-manager-podmonitor template accordingly.

cluster/charts/crossplane/templates/podmonitor.yaml (1)

8-11: ⚡ Quick win

Consider adding an additionalLabels hook for Prometheus CR label selection flexibility.

Many Prometheus CRs use custom podMonitorSelector label matchers beyond release: <name>. The release: {{ .Release.Name }} label already covers the default kube-prometheus-stack setup, but for non-standard configurations users have no way to attach extra labels without forking the template.

A lightweight addition to values.yaml + both templates would cover this:

♻️ Suggested change

In values.yaml:

     metricRelabelings: []
+    # -- Additional labels to add to the PodMonitor metadata.
+    # Useful when your Prometheus CR uses a custom podMonitorSelector.
+    additionalLabels: {}

In podmonitor.yaml and rbac-manager-podmonitor.yaml metadata:

   labels:
     app: {{ template "crossplane.name" . }}
     release: {{ .Release.Name }}
     {{- include "crossplane.labels" . | indent 4 }}
+    {{- with .Values.metrics.podMonitor.additionalLabels }}
+    {{- toYaml . | nindent 4 }}
+    {{- end }}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cluster/charts/crossplane/templates/podmonitor.yaml` around lines 8 - 11, The
PodMonitor metadata currently hardcodes labels using the release label and
included crossplane labels (see template "crossplane.name" and include
"crossplane.labels"); add support for a user-provided map (e.g.,
values.crossplane.podMonitor.additionalLabels) so consumers can inject arbitrary
extra labels for Prometheus podMonitorSelector matching. Update values.yaml to
declare the additionalLabels map, then modify podmonitor.yaml and
rbac-manager-podmonitor.yaml to merge that map into metadata.labels (rendering
nothing if unset) alongside the existing release and included labels so
non-standard Prometheus selectors can match.

cluster/charts/crossplane/values.yaml (1)

190-211: ⚡ Quick win

New podMonitor block looks well-structured and opt-in — nice work!

The defaults are sensible (enabled: false, empty optional fields, honorLabels: false). One thing worth a brief comment in the values file: when tlsConfig is configured, scheme must also be set to https. Currently the two fields have no cross-reference, which could cause silent scrape failures for users who set tlsConfig but leave scheme: http.

📝 Suggested comment addition

     # -- HTTP scheme to use for scraping (http or https).
+    # When tlsConfig is configured, set this to "https".
     scheme: http

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cluster/charts/crossplane/values.yaml` around lines 190 - 211, Add a short
inline note in the podMonitor block explaining that if podMonitor.tlsConfig is
provided the podMonitor.scheme must be set to "https" (otherwise scraping can
fail silently); reference the podMonitor.tlsConfig and podMonitor.scheme keys in
the comment so users configure both together and consider suggesting validation
in the chart templates if possible (e.g., ensure scheme is https when tlsConfig
is non-empty).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

cluster/charts/crossplane/templates/rbac-manager-podmonitor.yaml (1)

22-22: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Same port: metrics named-port concern as in podmonitor.yaml.

The verification script raised on podmonitor.yaml line 22 covers this file too — please confirm rbac-manager-deployment.yaml also exposes a container port named metrics. If it doesn't, neither PodMonitor will scrape anything.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cluster/charts/crossplane/templates/rbac-manager-podmonitor.yaml` at line 22,
The PodMonitor in rbac-manager-podmonitor.yaml references port: metrics but the
rbac-manager Deployment may not expose a container port with the name "metrics",
so update the Deployment (rbac-manager-deployment.yaml) to expose a named
containerPort "metrics" (or change the PodMonitor to point at the actual port
name/number used); specifically, either add a containerPort block with name:
"metrics" and the corresponding port number in the rbac-manager container spec,
or update the PodMonitor's port field to match the existing container port
name/number so scraping will work.

cluster/charts/crossplane/templates/podmonitor.yaml (2)

23-23: 💤 Low value

| default "/metrics" is redundant here (and in rbac-manager-podmonitor.yaml)

values.yaml already sets path: /metrics as the default, so | default "/metrics" will only ever trigger if someone explicitly sets path: "". If you'd like to leave it as belt-and-suspenders for resilience that's totally fine, but it could be cleaned up for consistency with how interval, scrapeTimeout, and scheme are handled (no fallback defaults in the templates).

🔧 Optional: drop the redundant default

-      path: {{ .Values.metrics.podMonitor.path | default "/metrics" }}
+      path: {{ .Values.metrics.podMonitor.path }}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cluster/charts/crossplane/templates/podmonitor.yaml` at line 23, The template
applies a redundant Helm default for the metrics path; remove the pipe-default
expression so the template simply uses .Values.metrics.podMonitor.path (and do
the same change in rbac-manager-podmonitor.yaml) to rely on the value set in
values.yaml; locate occurrences of the expression
".Values.metrics.podMonitor.path | default \"/metrics\"" in podmonitor.yaml and
rbac-manager-podmonitor.yaml and replace them with
".Values.metrics.podMonitor.path" to keep template behavior consistent with
interval/scrapeTimeout/scheme.

---

1-1: Operational note: enabling both metrics.enabled and metrics.podMonitor.enabled can cause double scraping.

When metrics.enabled: true, the pods receive prometheus.io/scrape: "true" annotations. If the user's Prometheus is also configured for annotation-based pod discovery, enabling a PodMonitor on top of this means each pod gets scraped twice — once by annotation discovery and once via the PodMonitor. Many Prometheus Operator users intentionally disable annotation-based discovery in favour of PodMonitors, so this may not bite everyone, but it's a footgun worth documenting in the values comments (e.g. "If using the Prometheus Operator, consider disabling annotation-based scraping in your Prometheus configuration to avoid double scraping").

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cluster/charts/crossplane/templates/podmonitor.yaml` at line 1, The chart
currently enables both annotation-based scraping and a PodMonitor when
.Values.metrics.enabled and .Values.metrics.podMonitor.enabled are true, which
can cause double-scraping; update the chart by adding a clear warning in
values.yaml next to metrics.enabled and metrics.podMonitor.enabled explaining
the double-scrape footgun (mention Prometheus Operator and annotation-based
discovery) and, optionally, add a defensive change in podmonitor.yaml/template
logic to avoid emitting prometheus.io/scrape annotations when
.Values.metrics.podMonitor.enabled is true (i.e., skip adding
prometheus.io/scrape annotation if .Values.metrics.podMonitor.enabled) so users
who enable both won’t get double-scraped.

cluster/charts/crossplane/values.yaml (1)

190-211: 💤 Low value

Thanks for the clean values layout with helm-docs comments — this looks well-structured!

One small question: the scheme field defaults to http (a truthy value), which means the {{- with .Values.metrics.podMonitor.scheme }} block in both PodMonitor templates will always emit scheme: http. Was this intentional, or would it be more flexible to leave scheme: "" as the default (so the field is omitted unless explicitly set, letting the Prometheus Operator use its own default)? Just wanted to make sure the design is intentional before flagging it.

🔧 Optional: default scheme to empty to let Prometheus fall back to its own default

-    # -- HTTP scheme to use for scraping (http or https).
-    scheme: http
+    # -- HTTP scheme to use for scraping (http or https). Omit or leave empty to let the Prometheus Operator use its default.
+    scheme: ""

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cluster/charts/crossplane/values.yaml` around lines 190 - 211, The
podMonitor.scheme default is set to "http" which causes the template block {{-
with .Values.metrics.podMonitor.scheme }} in the PodMonitor templates to always
emit scheme: http; change the default in values.yaml (podMonitor.scheme) to an
empty string "" so the with-block will be skipped unless the user explicitly
sets a scheme, and update the helm-docs comment to note that leaving it empty
defers to the Prometheus Operator default; check the PodMonitor template
references to ensure they rely on the with-block (no template changes required
if default is changed).

cluster/charts/crossplane/templates/podmonitor.yaml (1)

1-4: ⚡ Quick win

Add CRD capability guard to PodMonitor rendering.

Thanks for adding support for Prometheus monitoring—I want to flag a potential safety issue with the Helm upgrade flow. Both podmonitor.yaml and rbac-manager-podmonitor.yaml are currently gated only by values flags (metrics.enabled and metrics.podMonitor.enabled). If someone enables these features but the Prometheus Operator CRD isn't yet installed, Helm will attempt to create an invalid resource, which can break upgrades and rollbacks.

Adding a .Capabilities.APIVersions.Has check would make the behavior safer and more transparent—Helm will skip rendering the resource gracefully if the CRD isn't available, rather than failing mid-upgrade:

Suggested change

-{{- if and .Values.metrics.enabled .Values.metrics.podMonitor.enabled }}
+{{- if and .Values.metrics.enabled .Values.metrics.podMonitor.enabled (.Capabilities.APIVersions.Has "monitoring.coreos.com/v1/PodMonitor") }}

This aligns with the Helm best practice of guarding optional CRD-dependent resources. The same pattern should apply to both PodMonitor templates in the chart.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cluster/charts/crossplane/templates/podmonitor.yaml` around lines 1 - 4,
PodMonitor templates are only gated by values and will render invalid resources
if the Prometheus Operator CRD isn't installed; update the top-level conditional
in the PodMonitor templates (the PodMonitor resource rendering in
podmonitor.yaml and the other rbac-manager-podmonitor.yaml template) to also
check Helm capabilities by adding .Capabilities.APIVersions.Has
"monitoring.coreos.com/v1/PodMonitor" (e.g. change the existing "{{- if and
.Values.metrics.enabled .Values.metrics.podMonitor.enabled }}" to include the
.Capabilities check) so Helm skips rendering the PodMonitor resource when the
CRD is absent.