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

fix: do not delete non-existing dashboard #1504

Merged
merged 1 commit into from Apr 23, 2024
Merged

fix: do not delete non-existing dashboard #1504

merged 1 commit into from Apr 23, 2024

Conversation

pb82
Copy link
Collaborator

@pb82 pb82 commented Apr 23, 2024

fixes #1498

When a dashboard is not found, we should not try to delete it.

Copy link
Collaborator

@NissesSenap NissesSenap left a comment

Choose a reason for hiding this comment

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

LGTM

@NissesSenap
Copy link
Collaborator

NissesSenap commented Apr 23, 2024

@smuda could you try to verify if this solves your issue? I know it's hard since it's hard to reproduce.

@pb82
Copy link
Collaborator Author

pb82 commented Apr 23, 2024

It can also be verified in the following way:

  1. Create a dashboard
  2. Delete the dashboard in Grafana
  3. Now delete the dashboard crd

controllers/dashboard_controller.go Outdated Show resolved Hide resolved
@weisdd
Copy link
Collaborator

weisdd commented Apr 23, 2024

Verified the same way as @pb82 suggested, saw the same traces as @smuda. After trying a build from this branch, the operator no longer crashes. So, I think that was it :)
Great job @pb82 :)

@weisdd weisdd merged commit e0d3ad8 into master Apr 23, 2024
10 checks passed
@weisdd weisdd deleted the fix-1498 branch April 23, 2024 14:36
@smuda
Copy link
Contributor

smuda commented Apr 23, 2024

Awesome, nicely found and quick fix!

What do you think about a bugfix release on 5.8 with at least this one and #1482 / #1500?

@weisdd
Copy link
Collaborator

weisdd commented Apr 23, 2024

@smuda During today's meeting, @pb82 mentioned that he can cut a release ~this Thursday.

@smuda
Copy link
Contributor

smuda commented Apr 23, 2024

Sounds great!

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.

[Bug] Go panic when syncing many dashboards
4 participants