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

Add extensive templating helpers based on Masterminds/sprig #3726

Open
oncilla opened this issue Feb 16, 2024 · 7 comments
Open

Add extensive templating helpers based on Masterminds/sprig #3726

oncilla opened this issue Feb 16, 2024 · 7 comments

Comments

@oncilla
Copy link

oncilla commented Feb 16, 2024

When writing custom templates, the current set of functions is fairly limited.

We could register the functions from https://github.com/Masterminds/sprig to offer the template authors an extensive set of helper functions. Docs of the functions is available here: https://masterminds.github.io/sprig/

If you are interested, I would be happy to send a patch.

TheMeier added a commit to TheMeier/alertmanager that referenced this issue Mar 20, 2024
TheMeier added a commit to TheMeier/alertmanager that referenced this issue Mar 20, 2024
fixes prometheus#3726
fixes prometheus#3762
fixes prometheus#603

Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
TheMeier added a commit to TheMeier/alertmanager that referenced this issue Mar 20, 2024
fixes prometheus#3726
fixes prometheus#3762
fixes prometheus#603

Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
TheMeier added a commit to TheMeier/alertmanager that referenced this issue Mar 20, 2024
fixes prometheus#3726
fixes prometheus#3762
fixes prometheus#603

Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
TheMeier added a commit to TheMeier/alertmanager that referenced this issue Mar 22, 2024
fixes prometheus#3726
fixes prometheus#3762
fixes prometheus#603

Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
TheMeier added a commit to TheMeier/alertmanager that referenced this issue Mar 22, 2024
fixes prometheus#3726
fixes prometheus#3762
fixes prometheus#603

Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
TheMeier added a commit to TheMeier/alertmanager that referenced this issue Mar 24, 2024
fixes prometheus#3726
fixes prometheus#3762
fixes prometheus#603

Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
TheMeier added a commit to TheMeier/alertmanager that referenced this issue Mar 24, 2024
fixes prometheus#3726
fixes prometheus#3762
fixes prometheus#603

Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
TheMeier added a commit to TheMeier/alertmanager that referenced this issue Mar 24, 2024
fixes prometheus#3726
fixes prometheus#3762
fixes prometheus#603

Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
TheMeier added a commit to TheMeier/alertmanager that referenced this issue Mar 24, 2024
fixes prometheus#3726
fixes prometheus#3762
fixes prometheus#603

Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
TheMeier added a commit to TheMeier/alertmanager that referenced this issue Apr 27, 2024
fixes prometheus#3726
fixes prometheus#3762
fixes prometheus#603

Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
@mhulscher
Copy link

A thumbsup does to not do justice to how badly I want this to be added.

TheMeier added a commit to TheMeier/alertmanager that referenced this issue May 3, 2024
fixes prometheus#3726
fixes prometheus#3762
fixes prometheus#603

Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
TheMeier added a commit to TheMeier/alertmanager that referenced this issue May 8, 2024
fixes prometheus#3726
fixes prometheus#3762
fixes prometheus#603

Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
TheMeier added a commit to TheMeier/alertmanager that referenced this issue May 8, 2024
fixes prometheus#3726
fixes prometheus#3762
fixes prometheus#603

Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
TheMeier added a commit to TheMeier/alertmanager that referenced this issue Jun 14, 2024
fixes prometheus#3726
fixes prometheus#3762
fixes prometheus#603

Signed-off-by: Christoph Maser <christoph.maser+github@gmail.com>
@grobinson-grafana
Copy link
Contributor

I'll share what I wrote in #3770:

I've spent some time thinking about this and my preference is that we don't add Sprig. Instead, we implement template functions as needed. We can also implement the most common functions in prometheus/common so they can be used in both Prometheus and Alertmanager.

The reasons I don't think we should add Sprig are the following:

  1. We need to maintain an allow list of functions that we consider safe, and disable functions that do filesystem operations, network calls, etc.
  2. Sprig adds 11 new dependencies to Alertmanager. I would like to reduce the number of dependencies we have over time, not increase them.
  3. We need to track Sprig for updates and vet changes to existing functions.
  4. If Sprig is ever deprecated or archived we need to implement every single function ourselves anyway, because users now depend on them.

And just to make sure there is no misunderstanding, I think we do need to add more useful functions to templates. But I think we should add our own, and not be dependent on another package for it (whether Sprig or something else).

@oncilla
Copy link
Author

oncilla commented Jun 17, 2024

@grobinson-grafana makes sense.

Would it make sense to collect a subset of "safe" sprig functions that we could add as an initial set?

@grobinson-grafana
Copy link
Contributor

Hi! 👋 Please feel free to put together a list of template functions that you think are missing. These shouldn't be functions copied from Sprig verbatim, but think about the kind of functions you are missing when writing templates in Alertmanager. It might be easier to pretend that Sprig doesn't exist 🙂

@TheMeier
Copy link
Contributor

@oncilla that approach is already implemented in proposed PR.

@oncilla
Copy link
Author

oncilla commented Jun 18, 2024

@TheMeier ack. But AFAICS, you are still importing sprig. If I understand @grobinson-grafana correctly, he wants to have an original implementation in alertmanager code, rather than importing it.

(I'm not agreeing with the point FWIW)

@TheMeier
Copy link
Contributor

@oncilla I misunderstood your comment, makes sense now

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 a pull request may close this issue.

4 participants