[Dashboard] Embeddable Migrations for By Value Panels #99715
Conversation
⏳ Build in-progress, with failures
Failed CI StepsTo update your PR or re-run it, just comment with: |
ThomThomson
force-pushed
the
feature/byValueMigrations
branch
from
b389022 to
ed47c6e
Compare
ThomThomson
added
Feature:Dashboard
ThomThomson
force-pushed
the
feature/byValueMigrations
branch
from
ed47c6e to
40f3e5b
Compare
|
Pinging @elastic/kibana-presentation (Team:Presentation) |
|
@elasticmachine merge upstream |
|
@elasticmachine merge upstream |
ThomThomson
added
the
auto-backport
| @@ -125,4 +127,13 @@ export class EmbeddableServerPlugin implements Plugin<EmbeddableSetup, Embeddabl | |||
| } | |||
| ); | |||
| }; | |||
|
|
|||
| private getMigrationVersions = () => { | |||
| const uniqueVersions = new Set<string>(); | |||
There was a problem hiding this comment.
you need to include base embeddable migrations and extention migrations into this, check embeddable migrate function (in the common)
There was a problem hiding this comment.
Good call! Thank you. I've added them in
| ): SavedObjectMigrationMap => { | ||
| const embeddableMigrations = deps.embeddable | ||
| .getMigrationVersions() | ||
| .filter((version) => semver.gt(version, '7.12.0')) |
There was a problem hiding this comment.
Do we even need to filter here, since there aren't any embeddable migrations before 7.12?
There was a problem hiding this comment.
Hum, that is a good point. It could potentially work either way, but maybe it's worth leaving it in just in case an embeddable migration gets added to an older version?
There was a problem hiding this comment.
we are not maintaining older versions so i don't think we'll do that
| '7.3.0': flow(migrations730), | ||
| '7.9.3': flow(migrateMatchAllQuery), | ||
| '7.11.0': flow(createExtractPanelReferencesMigration(deps)), | ||
| ...Object.fromEntries(embeddableMigrations), |
There was a problem hiding this comment.
Is there any reason to be concerned about the order that these might run in?
I'm thinking about like if we have a dashboard migration and an embeddable migration for the same version is it going to matter the order that they run in? I don't think it's going to matter, just wanted to bring it up though.
There was a problem hiding this comment.
Actually this brings up a very important consideration. If a developer adds a saved object migration for dashboard after the embeddable migrations, they could potentially overwrite the embeddable migrations for that version. I've left a comment explaining this, and providing an example of how to work around it. Thanks for bringing this up!
There was a problem hiding this comment.
Oh yeah, that's a great point.
What about we write a function that wraps the dashboard migrations with the embeddable migrations to avoid that.
So it becomes something like
return wrapWithEmbeddableMigrations({
'6.7.2': flow(...),
'7.0.0': flow(...),
...
}, embeddableMigrations)
There was a problem hiding this comment.
i agree this is a problem. what about moving dashboard migrations to dashboard embeddable ?
There was a problem hiding this comment.
Either of these suggested solutions would work I believe - I would be hesitant to choose one and implement it in this PR. Perhaps we could do a follow-up PR to make sure we choose the option which will make the DX of adding new dashboard migrations the easiest.
… embeddable migrations
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
History
To update your PR or re-run it, just comment with: |
* Implemented embeddable by value migrations
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
* Implemented embeddable by value migrations Co-authored-by: Devon Thomson <[email protected]>
* Implemented embeddable by value migrations
* Implemented embeddable by value migrations
Summary
Implements a new dashboard migration which runs for all versions that have at least one embeddable migration registered.
This migration loops through all the panels on the dashboard saved object, and if they are by value, it runs them through the embeddable migrate function.
Fixes #98173
Checklist
Delete any items that are not applicable to this PR.
For maintainers