Skip to content

Allow the creation of silences with pre-set IDs #67

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

Merged
merged 2 commits into from
Mar 21, 2024

Conversation

santihernandezc
Copy link

@santihernandezc santihernandezc commented Mar 18, 2024

This PR adds an Upsert option to Silences.Set() to create new silences with pre-set IDs. Instead of returning ErrNotFound, we create a silence using that ID. This is useful when replicating silences between the internal and remote Alertmanager (e.g. if we need to delete the same silence in both Alertmanagers).

@gotjosh
Copy link
Collaborator

gotjosh commented Mar 19, 2024

I don't think we need to modify the fork, we have access to the underlying silences struct directly in https://github.com/grafana/alerting/blob/main/notify/silences.go

@santihernandezc
Copy link
Author

@gotjosh we can add the ID to the silence struct before creating it, but if we try to create a silence with a predefined ID the Alertmanager will return ErrNotFound. With this PR we just need to change our current call to .Set() for a call to .Upsert()

@santihernandezc santihernandezc force-pushed the santihernandezc/add_upsert_silence_method branch from 4070531 to e2f2ed9 Compare March 19, 2024 15:24
@santihernandezc santihernandezc changed the title Add Upsert method to Silences Allow the creation of silences with pre-set IDs Mar 20, 2024
@santihernandezc santihernandezc force-pushed the santihernandezc/add_upsert_silence_method branch from c35d1f4 to e2f2ed9 Compare March 21, 2024 09:44
Copy link
Collaborator

@gotjosh gotjosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

PD: Why is the CI failing?

@santihernandezc
Copy link
Author

@gotjosh I checked and it has failed on the same step for the last few PRs, I'll submit an issue to fix it

@santihernandezc santihernandezc merged commit 40158de into main Mar 21, 2024
@grobinson-grafana
Copy link

When cherry-picking this PR, please make sure to include the added test coverage here.

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.

3 participants