Dashboard V0->V1 Migration: Schema migration v39 #99631
Conversation
ivanortegaalba
requested review from
axelavargas,
toddtreece,
dprokop and
kaydelaney
| @@ -55,3 +55,21 @@ func TestGetSchemaVersion(t *testing.T) { | |||
| }) | |||
| } | |||
| } | |||
|
|
|||
| type migrationTestCase struct { | |||
There was a problem hiding this comment.
I moved it here, to be reusable by the different vXX_test.go files.
| "transformations": [ | ||
| { | ||
| "id": "timeSeriesTable", | ||
| "options": { | ||
| "refIdToStat": { | ||
| "A": "mean", | ||
| "B": "max" | ||
| } | ||
| } | ||
| } | ||
| ], |
There was a problem hiding this comment.
This is the transformation that should be migrated
| "transformations": [ | ||
| { | ||
| "id": "timeSeriesTable", | ||
| "options": { | ||
| "A": { | ||
| "stat": "mean" | ||
| }, | ||
| "B": { | ||
| "stat": "max" | ||
| } | ||
| } | ||
| } | ||
| ], | ||
| "type": "timeseries" | ||
| } | ||
| ], |
There was a problem hiding this comment.
This is the expected result
There was a problem hiding this comment.
Should this file be created?
There was a problem hiding this comment.
do you mean automatically? We can, but I think it may be better to compare to real input/output values from the typescript -- otherwise we run the risk of testing the behavior we wrote, not our target 😬
There was a problem hiding this comment.
Ups, sorry to make sure I understood.
The input value is a dashboard that could be fetched from the DB in version X. For example, in case pkg/apis/dashboard/migration/testdata/output/39.refresh_true.39.json, the input file is in version 39, and it will not change after the v39 migration is executed.
However, I experienced that the output is automatically generated, so I assumed it is a "Snapshot testing" where we create the output the first time and ensure it is the correct one. Then, it stays there to make sure no other migration breaks it.
In this context, why introduce the "input/output from typescript" here? Should we add the output JSON file as the result of the DashboardMigrator.updateSchema()? I was using the output as the result of the migration, it will be compared against for following test runs.
There was a problem hiding this comment.
The behavior is entirely our choice and whatever makes this easiest/best to develop+maintain :)
The line that generates the snapshot is:
https://github.com/grafana/grafana/blob/main/pkg/apis/dashboard/migration/migrate_test.go#L73
we can choose to keep that or not
ivanortegaalba
added
no-backport
I took #99392 as a reference.
I ported the migration for v39. This migration updates the configuration of the
timeSeriesTabletransformation to support new options. This is the PR implementing these changes in the JS migrator.I'm not using DashboardSpec in Go since this would involve a lot of typing, which is not available nowadays. I don't know how to face this issue for coming PRs. In the end, v1 should use a defined type.