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

feat: add tenantID template function #3758

Merged
merged 11 commits into from
Jan 4, 2023

Conversation

FUSAKLA
Copy link
Contributor

@FUSAKLA FUSAKLA commented Dec 16, 2022

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

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Dec 21, 2022

@gotjosh PTAL if you find a while since you have the context 🙏 (no rush just so it does not stay forgotten)

@pracucci pracucci self-requested a review December 22, 2022 17:28
Copy link
Collaborator

@pracucci pracucci left a 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
Copy link
Collaborator

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?

Copy link
Contributor Author

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 :/

pkg/alertmanager/alertmanager.go Outdated Show resolved Hide resolved
pkg/alertmanager/api.go Outdated Show resolved Hide resolved
@pracucci
Copy link
Collaborator

@FUSAKLA I pushed a commit to add Webex support to Mimir. It was easier to do than explaining it 😉 I hope you don't mind!
41c55ad

FUSAKLA and others added 6 commits January 3, 2023 14:23
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>
Signed-off-by: Martin Chodur <m.chodur@seznam.cz>
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Jan 3, 2023

@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.
There is still the question of the vendor updates, which I'm not sure how to deal with.

I also added a note about the templating to the docs in dceb21c
image

PTAL

@FUSAKLA FUSAKLA requested a review from pracucci January 3, 2023 14:46
@FUSAKLA FUSAKLA marked this pull request as ready for review January 3, 2023 14:47
@FUSAKLA FUSAKLA requested review from a team as code owners January 3, 2023 14:47
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Signed-off-by: Marco Pracucci <marco@pracucci.com>
Copy link
Collaborator

@pracucci pracucci left a 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 🤗

@pracucci pracucci enabled auto-merge (squash) January 4, 2023 08:05
@pracucci pracucci merged commit 7d562bf into grafana:main Jan 4, 2023
@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Jan 4, 2023

Great, thanks!

@gotjosh
Copy link
Contributor

gotjosh commented Jan 9, 2023

Apologies for being late to the part but this looks very good to me! Thank you for your contribution @FUSAKLA.

@FUSAKLA
Copy link
Contributor Author

FUSAKLA commented Jan 10, 2023

No worries @gotjosh, thank you for guiding the AM work. That made it way easier :)

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.

Alerting: Inject tenant info into alert
3 participants