Skip to content

Conversation

ba-work
Copy link
Contributor

@ba-work ba-work commented Oct 1, 2025

What this PR does / why we need it

This PR enables the user to explicitly set a Prometheus CRD's spec.ruleSelector value to null. This is a supported/documented value that prevents the operator from importing any PrometheusRules. You can check this with kubectl explain prometheus.spec.ruleSelector or see it documented here

I encountered a situation where this is required (CRs created and validated, but not imported), and this chart doesn't allow explicitly setting prometheus.prometheusSpec.ruleSelector=null; currently it always coalesces to {}.

I made it also require prometheus.prometheusSpec.ruleSelectorNilUsesHelmValues=false since that seemed like the most intuitive and backwards compatible with current behavior.

Tested with:

helm install -g charts/kube-prometheus-stack \
  --set prometheus.prometheusSpec.ruleSelectorNilUsesHelmValues=false \
  --set prometheus.prometheusSpec.ruleSelector=null

Special notes for your reviewer

Checklist

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

Copy link
Member

@jkroepke jkroepke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ba-work and others added 4 commits October 3, 2025 08:50
Signed-off-by: Brock Alberry <brock.alberry@cse-cst.gc.ca>
Signed-off-by: Brock Alberry <brock.alberry@cse-cst.gc.ca>
Signed-off-by: Brock Alberry <brock.alberry@cse-cst.gc.ca>
Signed-off-by: Brock Alberry <brock.alberry@cse-cst.gc.ca>
@ba-work ba-work force-pushed the null-rule-selector branch from 7f93f9d to 1f63d34 Compare October 3, 2025 12:50
@ba-work
Copy link
Contributor Author

ba-work commented Oct 3, 2025

Could you please extend the unit tests in that case, maybe start by copy this file:

main/charts/kube-prometheus-stack/unittests/prometheus/scrape_config_selector_test.yaml

done!

@ba-work ba-work force-pushed the null-rule-selector branch from 43b84df to ed3fb09 Compare October 3, 2025 13:29
Signed-off-by: Brock Alberry <brock.alberry@cse-cst.gc.ca>
@ba-work ba-work force-pushed the null-rule-selector branch from ed3fb09 to cc66cbc Compare October 3, 2025 13:44
Signed-off-by: Brock Alberry <61976254+ba-work@users.noreply.github.com>
@ba-work ba-work requested a review from jkroepke October 3, 2025 14:22
@ba-work
Copy link
Contributor Author

ba-work commented Oct 3, 2025

for anyone that happens to stumble on this before its merged or closed, turns out you can get the same behavior but it relies on the way the operator currently handles on config precedence: if there are keys with the same name, the latter wins.

so that means, as of this comment, the following will result ruleSelector: null when the CR is actually applied to the cluster:

helm install -g charts/kube-prometheus-stack \
  --set prometheus.prometheusSpec.ruleSelectorNilUsesHelmValues=false \
  --set prometheus.prometheusSpec.ruleSelector=null \
  --set prometheus.prometheusSpec.additionalConfig.ruleSelector=null

Really, this will make helm generate this:

---
# Source: kube-prometheus-stack/templates/prometheus/prometheus.yaml
apiVersion: monitoring.coreos.com/v1
kind: Prometheus
...
spec:
  ...
  ruleSelector: {}
  ...
  ruleSelector: null # this one wins

Signed-off-by: Jan-Otto Kröpke <mail@jkroepke.de>
@jkroepke jkroepke enabled auto-merge (squash) October 6, 2025 01:47
jkroepke
jkroepke previously approved these changes Oct 6, 2025
ba-work and others added 2 commits October 7, 2025 08:51
Signed-off-by: Brock Alberry <brock.alberry@cse-cst.gc.ca>
auto-merge was automatically disabled October 7, 2025 13:04

Head branch was pushed to by a user without write access

@ba-work
Copy link
Contributor Author

ba-work commented Oct 7, 2025

@jkroepke renovate-bot necessitated another version bump. sorry!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants