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 option to configure Mimir alertmanager #447

Draft
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

Itaykal
Copy link
Contributor

@Itaykal Itaykal commented May 17, 2024

fixes #387

This PR fixes some comments, namings and object specific errors (which shouldn't be this specific)

@Itaykal Itaykal requested a review from a team as a code owner May 17, 2024 13:35
@Itaykal Itaykal changed the title Feature Add option to configure Mimir alertmanager May 17, 2024
@Itaykal Itaykal marked this pull request as draft May 17, 2024 13:36
@malcolmholmes
Copy link
Contributor

Thanks for this contribution! We can look at it next week. In the meantime, there's a few lint type things noted in the PR. Can you review them?

@Itaykal
Copy link
Contributor Author

Itaykal commented May 17, 2024

Thanks for this contribution! We can look at it next week. In the meantime, there's a few lint type things noted in the PR. Can you review them?

Will do when possible, please note that the changes are not final, some tests are missing and some features like list -r are behaving in a weird manner.

Another important note that I think will need some further discussion here is that there's no name and/or uid for such object and I truly don't know how to handle it.

@malcolmholmes
Copy link
Contributor

Another important note that I think will need some further discussion here is that there's no name and/or uid for such object and I truly don't know how to handle it.

How did @theSuess handle it for the equivalent Grafana resource? I think the pattern is already there.

@Itaykal
Copy link
Contributor Author

Itaykal commented May 18, 2024

How did @theSuess handle it for the equivalent Grafana resource? I think the pattern is already there.

Good point, seems like the implementation is based on having a static name/uid for said object, I could try to align my implementation with theirs.

@Itaykal
Copy link
Contributor Author

Itaykal commented May 18, 2024

Another issue I'm currently having is that pkg/mimir/rules_test.go needs changing as I changed the mimir client, is this test even relevant? I see that the rules are being tested as part of the integration tests under integration/mimir/rules_test.go

@julienduchesne julienduchesne removed the request for review from a team June 19, 2024 12:00
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.

[Feature Request]: External Alertmanager Contact Point and Notification Policy Support
2 participants