[Lens] Migration from 7.7 #62879

wylieconlon · 2020-04-07T23:01:20Z

This change is a copy & paste of a migration that Lens needs in 7.7- the migration code is the same, but inline comments highlight the differences. This is the reference: #62877

elasticmachine · 2020-04-08T18:04:52Z

Pinging @elastic/kibana-app (Team:KibanaApp)

wylieconlon · 2020-04-08T18:07:27Z

x-pack/plugins/lens/server/migrations.test.ts

+ */
+
+import { migrations, RawLensSavedXYObject770 } from './migrations';
+import { SavedObjectUnsanitizedDoc } from 'src/core/server';


This line is the only difference from 7.7 in the tests

wylieconlon · 2020-04-08T18:07:55Z

x-pack/plugins/lens/server/saved_objects.ts

@@ -22,6 +23,7 @@ export function setupSavedObjects(core: CoreSetup) {
        uiCapabilitiesPath: 'visualize.show',
      }),
    },
+    migrations,


This line is slightly different from 7.7 because migrations are now registered to specific types

wylieconlon · 2020-04-08T18:08:14Z

x-pack/plugins/lens/server/migrations.ts

+ */
+
+import { cloneDeep } from 'lodash';
+import { SavedObjectUnsanitizedDoc } from 'src/core/server';


This import is slightly different as the migrations are now importing types from the server

pgayvallet · 2020-04-09T08:42:11Z

x-pack/plugins/lens/server/migrations.test.ts

+    const migrate = (doc: SavedObjectUnsanitizedDoc | RawLensSavedXYObject770) =>
+      migrations['7.7.0'](doc);
+


Why not just const migrate = migrations['7.7.0'] ?

On a higher level, that kind of specialized input type makes sense imho.

Maybe we should adapt

kibana/src/core/server/saved_objects/migrations/types.ts

Lines 42 to 45 in 8524303

export type SavedObjectMigrationFn = (

doc: SavedObjectUnsanitizedDoc,

context: SavedObjectMigrationContext

) => SavedObjectUnsanitizedDoc;

to

export type SavedObjectMigrationFn = < InputDocDocType extends SavedObjectUnsanitizedDoc = SavedObjectUnsanitizedDoc, MigratedDocType extends SavedObjectUnsanitizedDoc = SavedObjectUnsanitizedDoc >( doc: InputDocDocType, context: SavedObjectMigrationContext ) => MigratedDocType;

@rudolf WDYT?

Also I think you already stated in another PR that we did not really want to export the SavedObjectUnsanitizedDoc type. Can you confirm or infirm that? For their usage and the type check they are performing, I think they gonna need this type.

Yeah I agree, a more specific type can be really useful for making sure there's no exceptions in migration code. In the future, I imagine we would add saved object validation with kbn-config-schema or io-ts and then this type would provide 100% certainty that you won't get "value of undefined" exceptions.

There was a previous PR where the PR used SavedObjectUnsanitizedDoc instead of the SavedObjectMigrationFn type. Using SavedObjectMigrationFn makes it easier to change the API in the future, most likely for changes to context: SavedObjectMigrationContext.

In this PR I'm not sure if using SavedObjectUnsanitizedDoc is helpful either because it requires narrowing the type with a type guard. A migration can only ever receive documents of it's type (@pgayvallet correct me if I'm wrong), so this is actually a bug in core's types or just our types being so loose they're misleading.

So unless there's some value to using this type that I'm not seeing, I'd suggest we don't export or use SavedObjectUnsanitizedDoc in this PR.

Okay, I'm going to get rid of these type checks.

A migration can only ever receive documents of it's type (@pgayvallet correct me if I'm wrong),

You are right (...)

so this is actually a bug in core's types or just our types being so loose they're misleading.

The main issue regarding typings here is that there is no TS link between a SavedObjectsType (or even SavedObjectsType.mappings) and it's associated object's properties (SavedObject<Properties>) and even less with the raw type we are using for migrations SavedObjectUnsanitizedDoc, meaning that there is ATM no way to automatically infer the types for the migration function input or output.

This is why adding generic types to SavedObjectMigrationFn sounds to me like a quick win to allow plugin developers to specify the input/output without having to manually redefine the signature.

rudolf

I don't have a problem per se with using SavedObjectUnsanitizedDoc but not sure if it's adding value and if it's not I'd rather us not expose it.

rudolf · 2020-04-09T18:57:01Z

x-pack/plugins/lens/server/migrations.test.ts

+    const migrate = (doc: SavedObjectUnsanitizedDoc | RawLensSavedXYObject770) =>
+      migrations['7.7.0'](doc);
+


Yeah I agree, a more specific type can be really useful for making sure there's no exceptions in migration code. In the future, I imagine we would add saved object validation with kbn-config-schema or io-ts and then this type would provide 100% certainty that you won't get "value of undefined" exceptions.

There was a previous PR where the PR used SavedObjectUnsanitizedDoc instead of the SavedObjectMigrationFn type. Using SavedObjectMigrationFn makes it easier to change the API in the future, most likely for changes to context: SavedObjectMigrationContext.

In this PR I'm not sure if using SavedObjectUnsanitizedDoc is helpful either because it requires narrowing the type with a type guard. A migration can only ever receive documents of it's type (@pgayvallet correct me if I'm wrong), so this is actually a bug in core's types or just our types being so loose they're misleading.

So unless there's some value to using this type that I'm not seeing, I'd suggest we don't export or use SavedObjectUnsanitizedDoc in this PR.

rudolf · 2020-04-09T18:59:09Z

x-pack/plugins/lens/server/migrations.ts

+import { SavedObjectUnsanitizedDoc } from 'src/core/server';
+
+export interface RawLensSavedXYObject770 {
+  type: 'lens';


by changing type: string RawLensSavedXYObject770 is compatible with SavedObjectUnsanitizedDoc and then the type guards can be removed.

wylieconlon · 2020-04-13T19:25:15Z

@elasticmachine merge upstream

kibanamachine · 2020-04-13T21:07:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3452d07

History

💚 Build #39737 succeeded 9b93e03
💛 Build #39729 was flaky f900183
💔 Build #39695 failed e789020
💔 Build #39476 failed 54e087d

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

mbondyra · 2020-04-14T14:40:59Z

Tested with the same configurations as in the PR #62877

* [Lens] Migration from 7.7 * Fix types * Fix types in test * Add docs * Commit forgotten file * Remove extra types Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* [Lens] Migration from 7.7 * Fix types * Fix types in test * Add docs * Commit forgotten file * Remove extra types Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* alerting/alert-services-mock: (107 commits) removed unused import added alert services mock and use it in siem [Metrics UI] Refactor With* containers to hooks (elastic#59503) [NP] Migrate logstash server side code to NP (elastic#63135) Clicking cancel in saved query save modal doesn't close it (elastic#62774) [Lens] Migration from 7.7 (elastic#62879) [Lens] Fix bug where suggestions didn't use filters (elastic#63293) Task/linux events (elastic#63400) [Remote clusters] guard against usageCollection plugin if unav… (elastic#63284) [Uptime] Remove pings graphql (elastic#59392) Index Pattern Field class - factor out copy_field code for future typescripting (elastic#63083) [EPM] add/remove package in package settings page (elastic#63389) Adjust API authorization logging (elastic#63350) Revert FTR: add chromium-based Edge browser support (elastic#61684) (elastic#63448) [Event Log] Adds namespace into save objects (elastic#62974) document code splitting for client code (elastic#62593) Escape single quotes surrounded by double quotes (elastic#63229) [Endpoint] Update cli mapping to match endpoint package (elastic#63372) update in-app links to metricbeat configuration docs (elastic#63295) investigation notes field (documentation / metadata) (elastic#63386) ...

* [Lens] Migration from 7.7 * Fix types * Fix types in test * Add docs * Commit forgotten file * Remove extra types Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

wylieconlon added 3 commits April 7, 2020 18:59

[Lens] Migration from 7.7

54e087d

Fix types

ca51136

Merge remote-tracking branch 'origin/master' into lens/7.7-migration-2

ccdf919

wylieconlon added Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.8.0 v8.0.0 labels Apr 8, 2020

wylieconlon added the release_note:skip Skip the PR/issue when compiling release notes label Apr 8, 2020

wylieconlon requested review from timroes and mbondyra April 8, 2020 18:05

Fix types in test

e789020

wylieconlon commented Apr 8, 2020

View reviewed changes

wylieconlon marked this pull request as ready for review April 8, 2020 18:08

wylieconlon requested a review from a team as a code owner April 8, 2020 18:08

wylieconlon mentioned this pull request Apr 8, 2020

Lens visualization upgraded from 7.5 shows empty dimension panels and broken series names #61939

Closed

wylieconlon added 2 commits April 8, 2020 16:25

Add docs

f900183

Commit forgotten file

9b93e03

pgayvallet reviewed Apr 9, 2020

View reviewed changes

rudolf reviewed Apr 9, 2020

View reviewed changes

wylieconlon added 2 commits April 9, 2020 15:33

Merge remote-tracking branch 'origin/master' into lens/7.7-migration-2

ca9043e

Remove extra types

afc2a6d

pgayvallet approved these changes Apr 11, 2020

View reviewed changes

Merge branch 'master' into lens/7.7-migration-2

3452d07

mbondyra approved these changes Apr 14, 2020

View reviewed changes

wylieconlon merged commit 8489efc into elastic:master Apr 14, 2020

wylieconlon deleted the lens/7.7-migration-2 branch April 14, 2020 15:04

wylieconlon mentioned this pull request Apr 14, 2020

[7.x] [Lens] Migration from 7.7 (#62879) #63469

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Migration from 7.7 #62879

[Lens] Migration from 7.7 #62879

wylieconlon commented Apr 7, 2020 •

edited

Loading

elasticmachine commented Apr 8, 2020

wylieconlon Apr 8, 2020

wylieconlon Apr 8, 2020

wylieconlon Apr 8, 2020

pgayvallet Apr 9, 2020 •

edited

Loading

rudolf Apr 9, 2020

wylieconlon Apr 9, 2020

pgayvallet Apr 11, 2020

rudolf left a comment

rudolf Apr 9, 2020

rudolf Apr 9, 2020

wylieconlon commented Apr 13, 2020

kibanamachine commented Apr 13, 2020

mbondyra commented Apr 14, 2020

		const migrate = (doc: SavedObjectUnsanitizedDoc \| RawLensSavedXYObject770) =>
		migrations['7.7.0'](doc);

	export type SavedObjectMigrationFn = (
	doc: SavedObjectUnsanitizedDoc,
	context: SavedObjectMigrationContext
	) => SavedObjectUnsanitizedDoc;

[Lens] Migration from 7.7 #62879

[Lens] Migration from 7.7 #62879

Conversation

wylieconlon commented Apr 7, 2020 • edited Loading

elasticmachine commented Apr 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Apr 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rudolf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wylieconlon commented Apr 13, 2020

kibanamachine commented Apr 13, 2020

💚 Build Succeeded

History

mbondyra commented Apr 14, 2020

wylieconlon commented Apr 7, 2020 •

edited

Loading

pgayvallet Apr 9, 2020 •

edited

Loading