Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[chart] Fix condition for enabling selfServiceMonitor #137

Merged

Conversation

AlexLov
Copy link
Contributor

@AlexLov AlexLov commented May 23, 2024

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
Copy link
Owner

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
Copy link
Contributor Author

AlexLov commented May 30, 2024

Hey @ricoberger
Done

@ricoberger
Copy link
Owner

Thanks again for your contribution 🙂

@ricoberger ricoberger added the changelog: changed Something was changed or updated label May 31, 2024
@ricoberger ricoberger merged commit 67df348 into ricoberger:main May 31, 2024
1 check passed
@AlexLov AlexLov deleted the chart_fix_selfmonitor_enable_condition branch June 1, 2024 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: changed Something was changed or updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants