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

schema: Use generated dashboard model in frontend #55769

Merged
merged 46 commits into from
Dec 20, 2022

Conversation

sdboyer
Copy link
Contributor

@sdboyer sdboyer commented Sep 26, 2022

What this PR does / why we need it:

Now that #54816 is in, we're actually finally ready to start using the generated dashboard types in the frontend omg omg omg ❤️ 🎉 🎊 🎉 ❤️

I figured i'd open this PR that just swaps in the type to get the ball rolling. Simply doing so produces almost 200 errors to be fixed :)

Maybe we end up figuring out a way to chunk it down, and we can close this PR. But i figured this was the easiest way for everyone to be able to see the TODO list.

@sdboyer sdboyer added area/dashboard/schemas no-backport Skip backport of PR area/schema Related to canonical Grafana entity schemas labels Sep 26, 2022
@grafanabot
Copy link
Contributor

@grafanabot
Copy link
Contributor

@sdboyer
Copy link
Contributor Author

sdboyer commented Sep 29, 2022

Just updated this to make it a Partial<Dashboard>, at least initially. This reduces the set of errors to deal with down to 121 from 174 - slightly more tractable :)

With this change, on a quick scan, it looks like the only non-test file with errors is DashboardModel.ts itself. Almost every other test issue appears to be certain required fields being absent from panels. Seems like something that the exported defaultPanel const could probably help mitigate?

@ivanortegaalba ivanortegaalba requested review from a team as code owners October 5, 2022 14:28
@ivanortegaalba ivanortegaalba requested a review from a team October 5, 2022 14:28
@ivanortegaalba ivanortegaalba requested a review from a team as a code owner October 5, 2022 14:28
@ivanortegaalba ivanortegaalba requested review from JoaoSilvaGrafana, academo, mdvictor, supilee, papagian, sh0rez and ying-jeanne and removed request for a team October 5, 2022 14:28
@grafanabot
Copy link
Contributor

Copy link
Contributor

@polibb polibb left a comment

Choose a reason for hiding this comment

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

The bug mentioned above is fixed!

I don't understand the breaking changes because the reason seems to be removed label, type, and name of VariableModel, when they are not removed in fact. Do you have any idea what is happening here with this check @ivanortegaalba?

@@ -66,6 +66,7 @@ describe('DashboardModel', () => {
panels: [
{
type: 'graph',
// @ts-expect-error
Copy link
Contributor

Choose a reason for hiding this comment

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

I took the liberty of moving this ts-expect-error here because I saw two errors from the typecheck:

public/app/features/dashboard/state/DashboardMigrator.test.ts:67:11 - error TS2578: Unused '@ts-expect-error' directive.

67           // @ts-expect-error
             ~~~~~~~~~~~~~~~~~~~

public/app/features/dashboard/state/DashboardMigrator.test.ts:70:13 - error TS2322: Type 'boolean' is not assignable to type '{ show: boolean; sort?: string | undefined; sortDesc?: boolean | undefined; }'.

70             legend: true,
               ~~~~~~

but now I sometimes see a fully successful pass of yarn typecheck and build the frontend successfully, and sometimes see some errors... I hope everything is alright, but please check one last time before you merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

I experienced this problem as well. it seems to be a problem of how TS updates the cached results. If you remove from your local tsconfig.tsbuildinfo you will see the type check success deterministically.

@ivanortegaalba
Copy link
Contributor

ivanortegaalba commented Dec 16, 2022

It is because now the list templating variable is optional, so we're "removing" it because it was mandatory before.

@ivanortegaalba
Copy link
Contributor

Here is a PR to fix the issues with Enterprise https://github.com/grafana/grafana-enterprise/pull/4255

@ivanortegaalba ivanortegaalba removed the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Dec 20, 2022
@github-actions github-actions bot removed this from the 9.4.0 milestone Dec 20, 2022
@sdboyer sdboyer reopened this Dec 20, 2022
@ivanortegaalba ivanortegaalba added this to the 9.4.0 milestone Dec 20, 2022
Copy link
Contributor

@ivanortegaalba ivanortegaalba left a comment

Choose a reason for hiding this comment

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

After multiple changes, and manual testing of the modified runtime code. This PR is ready! ✅ Waiting for a second approval and I'll merge

@grafanabot grafanabot added the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Dec 20, 2022
Copy link
Contributor

@polibb polibb left a comment

Choose a reason for hiding this comment

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

Ran all the checks and cannot find any manual bugs 🙌.

@ivanortegaalba ivanortegaalba merged commit f86abf0 into main Dec 20, 2022
@ivanortegaalba ivanortegaalba deleted the use-veneer-dashboard-model branch December 20, 2022 14:04
@dsotirakis dsotirakis modified the milestones: 9.4.0, 9.4.0-beta1 Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backend area/dashboard/schemas area/dashboard/templating area/explore area/frontend area/schema Related to canonical Grafana entity schemas enterprise-ok levitate breaking change A label indicating a breaking change and assigned by Levitate. no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

9 participants