Add integration test tracking changes performed on all SO types migration metadata #142306

pgayvallet · 2022-09-30T08:56:57Z

Summary

Add an integration test detecting when migration-related properties are updated on any SO type
Add internal docs describing what and how we should check when pinged for such reviews.

…mig-check

…-fix'

pgayvallet · 2022-09-30T12:13:42Z

packages/core/test-helpers/core-test-helpers-so-type-serializer/src/extract_migration_info.ts

+    name: soType.name,
+    namespaceType: soType.namespaceType,
+    convertToAliasScript: soType.convertToAliasScript,
+    convertToMultiNamespaceTypeVersion: soType.convertToMultiNamespaceTypeVersion,
+    migrationVersions,
+    schemaVersions,
+    mappings: getFlattenedObject(soType.mappings ?? {}),
+    hasExcludeOnUpgrade: !!soType.excludeOnUpgrade,


These are the fields used for the hashing.

Ideally we would use the 'content' of functions like excludeOnUpgrade or the registered migration functions, but functions can't safely be serialized because of the transpiler (also depends on the nodejs version), so we will have to do without it.

functions can't safely be serialized because of the transpiler

Yeah that's too bad -- we already pin to specific TS & Node versions and could theoretically update these hashes whenever we perform an upgrade, however I think Babel would be the main issue (we don't pin to a specific Babel version AFAIK).

We're probably fine having this be the one hole in our test... We've seen people do things like mess with already-shipped migration functions before, and it would be good to catch that, but at this point seems impractical to hold up this PR in an attempt to solve for that. This PR will still cover the vast majority of SO-related upgrade problems we've seen in the past.

pgayvallet · 2022-09-30T12:13:57Z

packages/core/test-helpers/core-test-helpers-so-type-serializer/src/get_migration_hash.ts

+  const hashParts = [
+    migInfo.name,
+    migInfo.namespaceType,
+    migInfo.convertToAliasScript ?? 'none',
+    migInfo.hasExcludeOnUpgrade,
+    migInfo.convertToMultiNamespaceTypeVersion ?? 'none',
+    migInfo.migrationVersions.join(','),
+    migInfo.schemaVersions.join(','),
+    JSON.stringify(migInfo.mappings, Object.keys(migInfo.mappings).sort()),
+  ];
+  const hashFeed = hashParts.join('-');


And this is the hashing logic.

pgayvallet · 2022-09-30T12:18:22Z

src/core/server/integration_tests/saved_objects/migrations/check_registered_types.test.ts

+    expect(hashMap).toMatchInlineSnapshot(`
+      Object {
+        "action": "7858e6d5a9f231bf23f6f2e57328eb0095b26735",
+        "action_task_params": "bbd38cbfd74bf6713586fe078e3fa92db2234299",
+        "alert": "48461f3375d9ba22882ea23a318b62a5b0921a9b",
+        "api_key_pending_invalidation": "9b4bc1235337da9a87ef05a1d1f4858b2a3b77c6",
+        "apm-indices": "ceb0870f3a74e2ffc3a1cd3a3c73af76baca0999",
+        "apm-server-schema": "2bfd2998d3873872e1366458ce553def85418f91",


So, we can't use dynamic snapshot easily to have one snapshot per type, at least not if we want to keep the snapshot inline (and even with non-inline, looping has its flaws)

So I had two options here:

use a single snapshot for all types

not use snapshot at all, and have a manually maintained map in the test field.

2. would have helped to have a more explicit error message (we could throw something like the comment at the top of the test), however the main upside I see with 1. is that we can eventually, as a follow-up, run this test with -u as a pre-run CI task (like it's already done for linting or codeowners file update). If we achieve this, it would avoid the PR author to do anything manually. CI would simply run the test with -u, update the snapshot, push the change, which would trigger our review.

This is for this reason (even if not implemented in current PR, we will have a follow-up for that) that I went with the 'big single snapshot' approach.

pgayvallet · 2022-09-30T12:18:56Z

src/core/server/docs/kib_core_reviewing_so_pr.mdx

+# Reviewing SavedObject PRs
+
+## How do SO type automatic review assignment works?
+


Input / comments / addition very welcome for this document.

elasticmachine · 2022-09-30T13:04:38Z

Pinging @elastic/kibana-core (Team:Core)

lukeelmers

Added a handful of nits/ideas for the docs. Feel free to take whichever ones you think are useful, and leave out the rest.

Otherwise the test itself LGTM and I think the approach is straightforward and should work well. It's a bummer we can't count on consistent hashes for serialized function bodies, but I don't think that would be an easy problem to solve, and doesn't feel like something we should delay this PR over.

lukeelmers · 2022-09-30T20:24:35Z

packages/core/test-helpers/core-test-helpers-so-type-serializer/src/extract_migration_info.ts

+    name: soType.name,
+    namespaceType: soType.namespaceType,
+    convertToAliasScript: soType.convertToAliasScript,
+    convertToMultiNamespaceTypeVersion: soType.convertToMultiNamespaceTypeVersion,
+    migrationVersions,
+    schemaVersions,
+    mappings: getFlattenedObject(soType.mappings ?? {}),
+    hasExcludeOnUpgrade: !!soType.excludeOnUpgrade,


functions can't safely be serialized because of the transpiler

Yeah that's too bad -- we already pin to specific TS & Node versions and could theoretically update these hashes whenever we perform an upgrade, however I think Babel would be the main issue (we don't pin to a specific Babel version AFAIK).

We're probably fine having this be the one hole in our test... We've seen people do things like mess with already-shipped migration functions before, and it would be good to catch that, but at this point seems impractical to hold up this PR in an attempt to solve for that. This PR will still cover the vast majority of SO-related upgrade problems we've seen in the past.

src/core/server/docs/kib_core_reviewing_so_pr.mdx

…mig-check

src/core/server/docs/kib_core_reviewing_so_type_pr.mdx

…mig-check

kibana-ci · 2022-10-06T10:20:04Z

💚 Build Succeeded

Buildkite Build
Commit: ecaedba

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/core-test-helpers-so-type-serializer`	-	12	+12

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-test-helpers-so-type-serializer`	-	13	+13

History

💔 Build #78604 failed 4ead937
💚 Build #77403 succeeded 93d3f4d
💛 Build #77129 was flaky 2e394df
💚 Build #77104 succeeded 2d83e58

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

…tion metadata (elastic#142306) * create empty test helper package * create extractor * create hash extraction logic * fix pkg names * actually create the test * updating README * [CI] Auto-commit changed files from 'node scripts/generate codeowners' * [CI] Auto-commit changed files from 'node scripts/eslint --no-cache --fix' * documentation draft * add more fields and update snapshot * update review doc * update review documentation * more feedback * updating snapshot Co-authored-by: kibanamachine <[email protected]>

pgayvallet and others added 10 commits September 29, 2022 12:49

create empty test helper package

545a200

create extractor

14ceb3b

create hash extraction logic

a497074

Merge remote-tracking branch 'upstream/main' into kbn-104100-so-type-…

2a3aa53

…mig-check

fix pkg names

65315ba

actually create the test

9e8f36e

updating README

ec57049

[CI] Auto-commit changed files from 'node scripts/generate codeowners'

e21f9d0

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

9dbf702

…-fix'

documentation draft

2d83e58

pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v8.6.0 labels Sep 30, 2022

pgayvallet added 2 commits September 30, 2022 13:31

add more fields and update snapshot

f07be15

update review doc

2e394df

pgayvallet commented Sep 30, 2022

View reviewed changes

pgayvallet marked this pull request as ready for review September 30, 2022 13:04

pgayvallet requested a review from a team as a code owner September 30, 2022 13:04

lukeelmers approved these changes Sep 30, 2022

View reviewed changes

pgayvallet added 2 commits October 3, 2022 10:32

Merge remote-tracking branch 'upstream/main' into kbn-104100-so-type-…

897d655

…mig-check

update review documentation

93d3f4d

rudolf reviewed Oct 3, 2022

View reviewed changes

src/core/server/docs/kib_core_reviewing_so_type_pr.mdx Show resolved Hide resolved

src/core/server/docs/kib_core_reviewing_so_type_pr.mdx Show resolved Hide resolved

src/core/server/docs/kib_core_reviewing_so_type_pr.mdx Outdated Show resolved Hide resolved

pgayvallet added 4 commits October 6, 2022 08:54

Merge remote-tracking branch 'upstream/main' into kbn-104100-so-type-…

ea629d4

…mig-check

more feedback

4ead937

Merge remote-tracking branch 'upstream/main' into kbn-104100-so-type-…

5fa1d86

…mig-check

updating snapshot

ecaedba

pgayvallet enabled auto-merge (squash) October 6, 2022 09:18

pgayvallet merged commit def43c4 into elastic:main Oct 6, 2022

kibanamachine added the backport:skip This commit does not require backporting label Oct 6, 2022

cnasikas mentioned this pull request Nov 24, 2022

[Cases] Ping the response-ops team whenever a new case attachment type is added in a PR #146252

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration test tracking changes performed on all SO types migration metadata #142306

Add integration test tracking changes performed on all SO types migration metadata #142306

pgayvallet commented Sep 30, 2022 •

edited

Loading

pgayvallet Sep 30, 2022

lukeelmers Sep 30, 2022

pgayvallet Sep 30, 2022

pgayvallet Sep 30, 2022

pgayvallet Sep 30, 2022

elasticmachine commented Sep 30, 2022

lukeelmers left a comment

lukeelmers Sep 30, 2022

kibana-ci commented Oct 6, 2022

API count

		# Reviewing SavedObject PRs

		## How do SO type automatic review assignment works?

Add integration test tracking changes performed on all SO types migration metadata #142306

Add integration test tracking changes performed on all SO types migration metadata #142306

Conversation

pgayvallet commented Sep 30, 2022 • edited Loading

Summary

pgayvallet Sep 30, 2022

Choose a reason for hiding this comment

lukeelmers Sep 30, 2022

Choose a reason for hiding this comment

pgayvallet Sep 30, 2022

Choose a reason for hiding this comment

pgayvallet Sep 30, 2022

Choose a reason for hiding this comment

pgayvallet Sep 30, 2022

Choose a reason for hiding this comment

elasticmachine commented Sep 30, 2022

lukeelmers left a comment

Choose a reason for hiding this comment

lukeelmers Sep 30, 2022

Choose a reason for hiding this comment

kibana-ci commented Oct 6, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

History

pgayvallet commented Sep 30, 2022 •

edited

Loading