-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Embeddables/migrations #82296
Embeddables/migrations #82296
Conversation
Code LGTM, I am pulling and trying to integrate with #80832 |
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.
Did a full pass on the code, in general makes sense, I just had a few questions on some areas I wasn't sure about.
export type PersistableStateDefinition<P extends SerializableState = SerializableState> = Partial< | ||
PersistableState<P> | ||
>; |
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.
Could you describe the changes here just a little bit more? I'm not 100% sure when you would want to use which type.
It looks like PersistableStateService
and PersistableState
are identical other than migrate
vs migrations
.
PersistableStateDefinition
is now just a shortcut way of calling Partial<PersistableState>
.
What would be the use case for when to use each? If all 3 are necessary maybe we can add some comments to clear this up.
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.
PersistableStateDefinition - is for definitions (embeddable factory, expression function etc), where things are mostly optional
PersistableState - actual items in the registry, where things are no longer optional but have defaults provided
PersistableStateService - service exposing a registry with persistable state items
yes, the main difference is that on definition thigns are optional, and the service exposes migrate function instead of migrations object
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.
Thanks for clarifying, makes sense.
My only minor nit then would be considering doing a shared interface for telemetry/extract/inject since they are the same, and then using the shared interface in PersistableState
and PersistableStateService
. But I don't feel too strongly on that; since they are already in the same file it should be pretty easy to remember to update it in both places if we ever need to
abb37cd
to
c90abc2
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.
|
||
return { | ||
state: updatedInput, | ||
references: refs, | ||
telemetry: getTelemetryFunction(commonContract), |
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.
Could we make this also available in setup phase because we will need this for server side SO migrations 🙏
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.
Code changes all LGTM! 🚀
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]@kbn/optimizer bundle module count
distributable file count
page load bundle size
History
To update your PR or re-run it, just comment with: |
* master: (127 commits) [ILM] Fix breadcrumbs (elastic#82594) [UX]Swap env filter with percentile (elastic#82246) Add platform's missing READMEs (elastic#82268) [Discover] Adding uiMetric to track Visualize link click (elastic#82344) [Search] Add used index pattern name to the search agg error field (elastic#82604) improve client-side SO client get pooling (elastic#82603) [Security Solution] Unskips Overview tests (elastic#82459) Embeddables/migrations (elastic#82296) [Enterprise Search] Refactor product server route registrations to their own files/folders (elastic#82663) Moving reinstall function outside of promise.all (elastic#82672) Load choropleth layer correctly (elastic#82628) Master backport elastic#81233 (elastic#82642) [Fleet] Allow snake cased Kibana assets (elastic#77515) Reduce saved objects authorization checks (elastic#82204) [data.search] Add request handler context and asScoped pattern (elastic#80775) [ML] Fixes formatting of fields in index data visualizer (elastic#82593) Usage collector readme (elastic#82548) [Lens] Visualization validation and better error messages (elastic#81439) [ML] Add annotation markers to time series brush area to indicate annotations exist outside of selected range (elastic#81490) chore(NA): install microdnf in UBI docker build only (elastic#82611) ...
Summary
adds migrate function to embeddable public api
each embeddable definition and embeddable enhancement can provide a list of migration functions per semver.
Checklist
Delete any items that are not applicable to this PR.
For maintainers