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

chore(GrafanaAlertRuleGroup): simplify finalize stage #1897

Merged

Conversation

Baarsgaard
Copy link
Collaborator

@Baarsgaard Baarsgaard commented Mar 13, 2025

Instead of removing individual alerts, the operator removed the entire group like it does for folders (presence of individual dashboards is not checked there).

@github-actions github-actions bot added the bugfix this PR fixes a bug label Mar 13, 2025
@Baarsgaard Baarsgaard marked this pull request as ready for review March 13, 2025 21:06
@Baarsgaard Baarsgaard force-pushed the fix_ignore_missing_alertrulegroups branch from 592d7f1 to a4d3a9f Compare March 14, 2025 18:09
Copy link
Collaborator

@weisdd weisdd left a comment

Choose a reason for hiding this comment

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

@Baarsgaard I think it does make sense to remove rule group if CR is deleted, but there's something I'd like to clarify before doing anything about this PR. - You mentioned that the operator gets stuck when trying to finalize CR is the alert group is missing, but I couldn't reproduce that. Moreover, the part of the code that you changed explicitly handles 404 (not found) scenario. - What am I missing here?

@Baarsgaard Baarsgaard force-pushed the fix_ignore_missing_alertrulegroups branch from cbab794 to 2882642 Compare March 20, 2025 20:36
@Baarsgaard Baarsgaard force-pushed the fix_ignore_missing_alertrulegroups branch from 2882642 to ab2b52b Compare March 20, 2025 20:37
@Baarsgaard Baarsgaard added chore and removed bugfix this PR fixes a bug labels Mar 20, 2025
@Baarsgaard
Copy link
Collaborator Author

Yeah, this was me late in the evening not doing my due diligence..
I essentially looked through the finalize functions and looked for the ones that did a Get first, this one especially I did not put any thought into...

This does not fix a bug, it's a refactor to achieve the same with less requests.

@Baarsgaard Baarsgaard changed the title Fix: Ignore missing GrafanaAlertRuleGroup on delete Chore(GrafanaAlertRuleGroup): simplify finalize stage Mar 20, 2025
@Baarsgaard Baarsgaard requested a review from weisdd March 20, 2025 20:51
@weisdd weisdd changed the title Chore(GrafanaAlertRuleGroup): simplify finalize stage chore(GrafanaAlertRuleGroup): simplify finalize stage Mar 21, 2025
@weisdd weisdd added this pull request to the merge queue Mar 21, 2025
Merged via the queue into grafana:master with commit 541b8bb Mar 21, 2025
15 checks passed
@Baarsgaard Baarsgaard deleted the fix_ignore_missing_alertrulegroups branch March 21, 2025 19:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants