-
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
Use absolute path of template to decide if we need to remove it #3604
Conversation
ece87b4
to
bf9b9ce
Compare
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.
Could this fix be accompanied by a test? I think that would be helpful in order to avoid a potential future regression :)
I'll need some guidance on how to write a test for this case. I can trigger it in |
Why are they? I think I need more guidance on what the root cause is, to better understand the fix. |
I was also hoping a test would make it clearer how this bug is happening. |
A list of all current file templates is built at https://github.com/grafana/mimir/blob/main/pkg/alertmanager/multitenant.go#L638 Then, at https://github.com/grafana/mimir/blob/main/pkg/alertmanager/multitenant.go#L649 they are removed from the list if they are still relevant (ie, in the received configuration). But at theses two steps, they aren't built in the same way :
My PR built the two filenames the same way, fixing the bug. |
f97f7a6
to
0520bbb
Compare
I've added a testcase in the PR |
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.
Test looks good to me! I'll wait for some other maintainers to take look before merging.
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, this looks good as I can understand why you would need to make this fix (to mirror line 643) in light of your test.
Please review my language related suggestions :)
0520bbb
to
076b234
Compare
Thanks for your suggestions :) Here they are. |
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.
LGTM, nice fix!
…ana#3604) * Test if templates are persisted across alertmanager configuration syncs * Use absolute path of template to decide if we need to remove it
What this PR does
When alertmanager.data_dir is set to a relative path (as in default configuration), templates are deleted and recreated on a regular basis.
This PR fix the issue.
Which issue(s) this PR fixes or relates to
Fixes #3591
Checklist
CHANGELOG.md
updated - the order of entries should be[CHANGE]
,[FEATURE]
,[ENHANCEMENT]
,[BUGFIX]