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

Update grafana.ini.j2 #203

Closed
wants to merge 2 commits into from
Closed

Update grafana.ini.j2 #203

wants to merge 2 commits into from

Conversation

twdrabek
Copy link

Changed the legacy setting [alerting] to [unified_alerting]. Grafana-server will not start with the legacy setting enabled.

Changed the legacy setting `[alerting]` to `[unified_alerting]`. Grafana-server will not start with the legacy setting enabled.
@CLAassistant
Copy link

CLAassistant commented May 16, 2024

CLA assistant check
All committers have signed the CLA.

@voidquark
Copy link
Collaborator

I think it would be good to rename the variable grafana_alerting to grafana_unified_alerting.

@twdrabek
Copy link
Author

I think it would be good to rename the variable grafana_alerting to grafana_unified_alerting.

I agree--for consistency. However, if I'm not mistaken, this is lower level. This is a section header in Grafana.ini so changing variable alone will not fix the error. If version selection allows a numerical version, it might be better to use a tag with a ternary to select the correct section header.

@intermittentnrg
Copy link

I quite like how the grafana helm chart automatically converts yaml to grafana.ini

Example config: https://github.com/grafana/helm-charts/blob/main/charts/grafana/values.yaml#L772-L1098
Templating: https://github.com/grafana/helm-charts/blob/main/charts/grafana/templates/_config.tpl#L11-L36

grafana.ini:
  alerting:
    enabled: true

becomes

[alerting]
enabled = true

Should be doable for ansible too. Maybe should be a separate issue tho.

@intermittentnrg
Copy link

intermittentnrg commented May 21, 2024

Created separate issue #210 for my suggestion above.

@voidquark
Copy link
Collaborator

I think it would be good to rename the variable grafana_alerting to grafana_unified_alerting.

I agree--for consistency. However, if I'm not mistaken, this is lower level. This is a section header in Grafana.ini so changing variable alone will not fix the error. If version selection allows a numerical version, it might be better to use a tag with a ternary to select the correct section header.

Version selection makes sense for backward compatibility, which I like. Could you implement this and update the PR?

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.

5 participants