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

Embeddables/migrations #82296

Merged
merged 5 commits into from
Nov 5, 2020
Merged

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Nov 2, 2020

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

@ppisljar ppisljar requested a review from a team as a code owner November 2, 2020 15:06
@botelastic botelastic bot added Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) labels Nov 2, 2020
@ppisljar ppisljar added release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0 labels Nov 2, 2020
@Dosant
Copy link
Contributor

Dosant commented Nov 3, 2020

Code LGTM, I am pulling and trying to integrate with #80832

Copy link
Member

@lukeelmers lukeelmers left a 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.

src/plugins/embeddable/server/plugin.ts Outdated Show resolved Hide resolved
src/plugins/embeddable/server/plugin.ts Outdated Show resolved Hide resolved
src/plugins/kibana_utils/common/semver_gte.ts Outdated Show resolved Hide resolved
Comment on lines +101 to +103
export type PersistableStateDefinition<P extends SerializableState = SerializableState> = Partial<
PersistableState<P>
>;
Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

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

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

LGTM,
I tested this on running embeddable references extract during dashboard SO migration on #82602
Didn't actually tried migrate part (because appeared that doesn't need that for #82602) just yet, but at least tested that inject/extract part is working.


return {
state: updatedInput,
references: refs,
telemetry: getTelemetryFunction(commonContract),
Copy link
Contributor

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 🙏

Copy link
Member

@lukeelmers lukeelmers left a 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! 🚀

@lukeelmers
Copy link
Member

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
embeddable 94 99 +5

distributable file count

id before after diff
default 42717 42722 +5
oss 22416 22421 +5

page load bundle size

id before after diff
embeddable 219.6KB 220.6KB +1012.0B
expressions 190.2KB 188.6KB -1.7KB
uiActionsEnhanced 304.3KB 304.4KB +59.0B
total -644.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@ppisljar ppisljar merged commit 7f70fc8 into elastic:master Nov 5, 2020
ppisljar added a commit to ppisljar/kibana that referenced this pull request Nov 5, 2020
ppisljar added a commit that referenced this pull request Nov 5, 2020
gmmorris added a commit to gmmorris/kibana that referenced this pull request Nov 5, 2020
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants