ricoberger / script_exporter

Prometheus exporter to execute scripts and collect metrics from the output or the exit status.
MIT License
354 stars 82 forks source link

[chart] Fix condition for enabling selfServiceMonitor #137

Closed AlexLov closed 5 months ago

AlexLov commented 6 months ago

I believe selfServiceMonitor shouldn't be enabled until serviceMonitor is also enabled. Otherwise it's better to move its configuration to separate/independent section and disable by default same way as serviceMonitor.

Otherwise I got the error:

no matches for kind "ServiceMonitor" in version "monitoring.coreos.com/v1" ensure CRDs are installed first

This is because I don't use it and thus didn't install this CRD at all but chart worked for me but failed when I tried to upgrade it from 0.6.2 to 0.8.3.

ricoberger commented 6 months ago

Hi @AlexLov and thanks for your contribution 🙂.

The .Values.serviceMonitor.enabled is used to create the ServiceMonitors for the scripts defined in the config file. So I think it would make more sense to change the default value for .Values.serviceMonitor.selfMonitor.enabled to false to work around the problem.

I'm with you that it makes more sense to move it to it's own section in this case, but looking at other Helm charts (e.g. the blackbox-exporter) it looks like they are handling this very similar:

So I think I would prefer to just change the default value to false for now. Can you adjust you PR accordingly please and also bump the Helm chart version in Chart.yaml file.

AlexLov commented 5 months ago

Hey @ricoberger Done

ricoberger commented 5 months ago

Thanks again for your contribution 🙂