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

Accept template contents as strings instead of reading from disk #161

Merged
merged 13 commits into from
Mar 4, 2024

Conversation

JacobsonMT
Copy link
Member

@JacobsonMT JacobsonMT commented Feb 26, 2024

What is this feature?

Makes use of the new upstream Template.New() to stop passing user-defined templates to the Grafana Alertmanager by persisting them to disk.

Why do we need this feature?

This PR is a general improvement and simplification, but there are also some specific reasons why we might want to do this:

  • Incremental step towards abstracting data layer for Grafana's Alertmanager interface away from needing disk-based storage. Next step would likely focus on FileStore.
  • Reduces likelihood of consistency complexities on save/apply, especially in the context of future Remote AM work.
  • Since DefaultTemplateString is now treated like other defaults and therefor consistently applied first. This ensures that user-defined templates can now rely on being able to override defaults.

Future improvements

  • Similarly, we could consider moving the existing prometheus defaults (default.tmpl and email.tmpl) to strings as well, allowing them to be more easily overridden via API/UI.
  • Similar changes can be made in Mimir Alertmanager eventually as well.

Copy link
Contributor

@grobinson-grafana grobinson-grafana left a comment

Choose a reason for hiding this comment

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

LGTM! A couple of comments.

templates/template_data.go Outdated Show resolved Hide resolved
templates/template_data.go Outdated Show resolved Hide resolved
templates/template_data.go Outdated Show resolved Hide resolved
notify/templates_test.go Show resolved Hide resolved
Copy link
Contributor

@yuri-tceretian yuri-tceretian left a comment

Choose a reason for hiding this comment

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

LGTM.

I do not think that there is anything else to do because we do not let users to override the files on the disc. According to the code https://github.com/grafana/grafana/blob/a564c8c4398fe2a763712fa54e21885017151b45/pkg/services/ngalert/notifier/config.go#L25-L54 we always override the content of the files.

Copy link
Collaborator

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

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

LGTM - my comments are nits and I don't need to look at this again.

notify/grafana_alertmanager.go Show resolved Hide resolved
notify/templates.go Show resolved Hide resolved
templates/template_data.go Outdated Show resolved Hide resolved
@JacobsonMT JacobsonMT merged commit e81931a into main Mar 4, 2024
3 checks passed
@JacobsonMT JacobsonMT deleted the jacobsonmt/in-memory-templates branch March 4, 2024 17:53
JacobsonMT added a commit to grafana/grafana that referenced this pull request Mar 4, 2024
Updates Grafana Alertmanager to work with new interface from grafana/alerting#161. This change stops passing user-defined templates to the Grafana Alertmanager by persisting them to disk and instead passes them by string.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants