-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
fixes embeddables migrate function #101470
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
3188923
to
35c3e73
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested locally by importing a dashboard from 7.12.1 which had a by value panel with a drilldown. The import went through, but unfortunately the drilldown was lost. This is because this migration function removes all enhancements which don't have migrations registered.
I'm pretty sure that this behaviour is unintentional. I left a fix in the comments, which I've tested locally. The fix works correctly, but I would strongly encourage you to verify it by testing this fix with an import.
const enhancementDefinition = embeddables.getEnhancement(key); | ||
if (enhancementDefinition.migrations && enhancementDefinition.migrations[version]) { | ||
(updatedInput.enhancements! as Record<string, any>)[key] = enhancementDefinition.migrations[ | ||
version | ||
](enhancements[key] as SerializableState); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this function. It should run migrations on enhancements which have them, and retain enhancements which don't.
const enhancementDefinition = embeddables.getEnhancement(key); | |
if (enhancementDefinition.migrations && enhancementDefinition.migrations[version]) { | |
(updatedInput.enhancements! as Record<string, any>)[key] = enhancementDefinition.migrations[ | |
version | |
](enhancements[key] as SerializableState); | |
} | |
const enhancementDefinition = embeddables.getEnhancement(key); | |
const migratedEnhancement = enhancementDefinition?.migrations?.[version] | |
? enhancementDefinition.migrations[version](enhancements[key] as SerializableState) | |
: enhancements[key]; | |
(updatedInput.enhancements! as Record<string, any>)[key] = migratedEnhancement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ppisljar - would you be able to add a test that would have caught this issue? Unless I'm missing it, it looks like the only test added was for the original bug, not the secondary bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the test was added to catch this as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks Peter!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's my understanding (maybe I'm wrong), that your first fix had a bug that Devon found and it wasn't caught by tests? That is the situation I am asking for tests for. Just because the original code didn't show that bug doesn't mean there shouldn't be a test for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because original code didn't show that bug you trying to take original code and get the new test failing doesn't work.
you would need to take the intermediate code (where devon found a bug) and then the test would fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I synced with @ppisljar about this, and we saw how if we take the first iteration of code (pasted below), and run the tests, they all still pass:
const enhancementDefinition = embeddables.getEnhancement(key);
if (enhancementDefinition.migrations && enhancementDefinition.migrations[version]) {
(updatedInput.enhancements! as Record<string, any>)[key] = enhancementDefinition.migrations[
version
](enhancements[key] as SerializableState);
}
@ppisljar - can you follow up when you've added a test that would catch the bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This definitely needs unit test coverage.
As some functional coverage for this issue, In this PR I assert that all drilldowns survive the migration process (on by value & by reference panels). The original code removed drilldowns from by value panels, so if we run into something like this again, the smoke test should fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed #102772 to keep track
Co-authored-by: Devon Thomson <devon.thomson@elastic.co>
…ibana into fix/embeddablemigrate
💚 Build SucceededMetrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix LGTM. Tested locally again, and the drilldown is properly coming through now. Thanks for implementing this!
* master: (173 commits) [kbnArchiver] convert archive names to root-relative paths (elastic#101839) [Reporting] Make "ScreenCapturePanel" shareable for Canvas (elastic#100623) [Alerting UI] Converted Rules and Connectors management pages to new layout. (elastic#101697) [Fleet] Support granular integrations in policy editor (elastic#101531) [Security Solution][Detections] Update detection alert mappings to ECS v1.10.0 (elastic#101680) [Fleet] Integrations UI: Adjust policies list UI (elastic#101600) chore(NA): moving @kbn/server-route-repository into bazel (elastic#101484) Support owner and description attributes inside the Manifest file, use in API docs (elastic#101786) [Security Solution] fix security empty overview links (elastic#101536) Unskips migration tests now that elastic search is fixed (elastic#101682) Fix endpoint -> integrations onboarding link (elastic#101804) [Alerting] Log warning when rules are not rescheduled due to Saved Object not found error (elastic#101591) Update datafeed_high_count_network_denies.json (elastic#101681) [Index patterns] Field editor example app (elastic#100524) [DOCS] Adding file upload to add data page (elastic#101674) [Security Solution][Endpoint] Adds Endpoint Host Isolation Status common component (elastic#101782) Upgrade ws v7.3.1->v7.4.2 and v6.2.1->v6.2.2 (elastic#101402) fixes embeddables migrate function (elastic#101470) [Canvas] Update slow query in sample ecommerce workpad (elastic#100714) clarify which parts of TM are experimental (elastic#101757) ...
Pinging @elastic/kibana-app-services (Team:AppServices) |
Summary
fixes
embeddables
migrate function to not expect an enhancement migration defined for every version of kibanaresolves #101430
Checklist