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.

Sign up for GitHub

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

Jump to bottom

PublicDashboards: Orphaned public dashboard deletion script added #57917

Merged
merged 5 commits into from Nov 22, 2022
Merged

PublicDashboards: Orphaned public dashboard deletion script added #57917

merged 5 commits into from Nov 22, 2022

Conversation

juanicabanas
Copy link
Contributor

@juanicabanas juanicabanas commented Oct 31, 2022 •
edited

What is this feature?

PublicDashboards: When a dashboard was deleted, public dashboard was not also deleted. There was already a change to delete also public dashboards, but there can be remaining orphaned public dashboards before this chain deletion which can appear in Audit table.

This feature makes a deletion on public dashboard table, for those who don't have a dashboard.

Why do we need this feature?

Deletes orphaned public dashboards.

Which issue(s) does this PR fix?:

Fixes #478

Special notes for your reviewer:

@juanicabanas juanicabanas added this to the 9.3.0 milestone Oct 31, 2022
@juanicabanas juanicabanas requested a review from a team October 31, 2022 16:03
@juanicabanas juanicabanas self-assigned this Oct 31, 2022
@juanicabanas juanicabanas requested review from a team as code owners October 31, 2022 16:03
@juanicabanas juanicabanas requested review from papagian, zserge and mildwonkey and removed request for a team October 31, 2022 16:03
@juanicabanas juanicabanas requested review from a team, evictorero and mmandrus and removed request for a team October 31, 2022 16:04
owensmallwood
owensmallwood approved these changes Oct 31, 2022
View reviewed changes
Copy link
Contributor

@owensmallwood owensmallwood left a comment

Choose a reason for hiding this comment

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

Bulk deletion migrations are scary. This is a great Halloween PR 😱

Tested it out just be sure, and works as expected. LGTM 👍

@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/40004

@jalevin jalevin self-requested a review October 31, 2022 16:17
jalevin
jalevin requested changes Oct 31, 2022
View reviewed changes
Copy link
Contributor

@jalevin jalevin left a comment

Choose a reason for hiding this comment

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

Let's hold off on this. Will discuss at standup

@jalevin
Copy link
Contributor

jalevin commented Oct 31, 2022

Discussion follow-up post standup.

We did not previously delete public dashboards when deleting a dashboard so it is possible that instances have orphaned public dashboards. This migration deletes those. We are also merging (today) the ability for a user to delete dashboards from the audit table. Orphaned dashboards will be shown as well with the ability to delete them.

@bergquist How do you feel about this destructive migration?

@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/40234

@leandro-deveikis leandro-deveikis modified the milestones: 9.3.0, 9.3.0-beta1 Nov 14, 2022
@grafanabot grafanabot removed this from the 9.3.0-beta1 milestone Nov 15, 2022
@grafanabot
Copy link
Contributor

This pull request was removed from the 9.3.0-beta1 milestone because 9.3.0-beta1 is currently being released.

owensmallwood
owensmallwood reviewed Nov 21, 2022
View reviewed changes
Copy link
Contributor

@owensmallwood owensmallwood left a comment

Choose a reason for hiding this comment

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

Did we decide to go ahead with this? Can't remember, it was so long ago lol

@jalevin
Copy link
Contributor

jalevin commented Nov 22, 2022

I followed up directly with @bergquist and @grafana/hosted-grafana-team via slack.

jalevin
jalevin approved these changes Nov 22, 2022
View reviewed changes
Copy link
Contributor

@jalevin jalevin left a comment

Choose a reason for hiding this comment

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

Per discussion with @hairyhenderson: There is no way to run this same sql across instances in cloud that are archived. We do have an alternative to do this in the UI, however, it's straight forward, basically idempotent, and an edge case that is covered moving forward, so we'll run this one.

@jalevin jalevin added this to the 9.3.0 milestone Nov 22, 2022
@jalevin jalevin merged commit a93b8a0 into main Nov 22, 2022
@jalevin jalevin deleted the juanicabanas/orphaned_public_dashboard branch November 22, 2022 22:11
@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/42966

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owensmallwood owensmallwood owensmallwood approved these changes

@jalevin jalevin jalevin approved these changes

@papagian papagian Awaiting requested review from papagian papagian was automatically assigned from grafana/backend-platform

@zserge zserge Awaiting requested review from zserge zserge was automatically assigned from grafana/backend-platform

@mildwonkey mildwonkey Awaiting requested review from mildwonkey mildwonkey was automatically assigned from grafana/backend-platform

@evictorero evictorero Awaiting requested review from evictorero evictorero was automatically assigned from grafana/grafana-operator-experience-squad

@mmandrus mmandrus Awaiting requested review from mmandrus mmandrus was automatically assigned from grafana/grafana-operator-experience-squad

Assignees

@juanicabanas juanicabanas

Labels
add to changelog area/backend/db/migration area/backend area/frontend effort/small enterprise-failed no-backport Skip backport of PR
Projects
None yet
Milestone
9.3.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@juanicabanas @grafanabot @jalevin @owensmallwood @leandro-deveikis