-
Notifications
You must be signed in to change notification settings - Fork 529
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
feat: add tenantID template function #3758
Conversation
@gotjosh PTAL if you find a while since you have the context 🙏 (no rush just so it does not stay forgotten) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this! Overall LGTM. I left a couple of minor comments. Can you also add a CHANGELOG entry and rebase please?
The vendored alertmanager update adds Webex support. It should be also mentioned in the CHANGELOG.
@@ -18,7 +18,7 @@ require ( | |||
github.com/gorilla/mux v1.8.0 | |||
github.com/grafana/dskit v0.0.0-20221216094413-a9826f9b0468 | |||
github.com/grafana/e2e v0.1.1-0.20221115065638-fe4609fcbc71 | |||
github.com/hashicorp/golang-lru v0.5.4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are many vendor updates here. Are all of these required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, it looks like it, or we would need to tweak it in the go.mod using some replace?
I suppose that was caused by the upgrade of the golang-lru in alertmanager :/
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
41c55ad
to
975295f
Compare
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
@pracucci not at all, thanks for that! Sorry about the delay, needed to take a break over holidays 🎄 Should be rebased now, added the changelog entry and resolved all the comments, thanks for looking into it. I also added a note about the templating to the docs in dceb21c PTAL |
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @FUSAKLA, LGTM! I just pushed a nit change to CHANGELOG and a smoke test in the config validation API. I hope you don't mind 🤗
Great, thanks! |
Apologies for being late to the part but this looks very good to me! Thank you for your contribution @FUSAKLA. |
No worries @gotjosh, thank you for guiding the AM work. That made it way easier :) |
As discussed in this issue #3121 and as a followup from prometheus/alertmanager#3174 using the additional options.
I suppose we will wait for some AM release, not to import random commits?
Ad tests, I wanted to do some more on point test, but it's hard to make the AM to just render a template :/
So at least testing the option.
closes #3121