Add CI check to ensure SO mapping addition are done correctly #172056

pgayvallet · 2023-11-28T12:43:54Z

Summary

Fix #172055

Add a CI check verifying that any mapping addition (done after a type's initial introduction) correctly defines the added mappings as a mappings_addition change in a model version of the owning type (or throws otherwise)

Similar to #169610

…I failure

…te --fix'

…ngs_update --fix'" This reverts commit 47ff997.

…gger a CI failure" This reverts commit 4f592c1.

…fix'

…-fix'

This reverts commit a50381f.

…apping-additions

pgayvallet

self-review

pgayvallet · 2023-11-29T19:23:36Z

packages/kbn-check-mappings-update-cli/src/compatibility/run_mappings_compatibility_check.ts

+export const runMappingsCompatibilityChecks = async ({
+  fix,
+  verify,
+  log,


Extracted from packages/kbn-check-mappings-update-cli/src/run_check_mappings_update_cli.ts

pgayvallet · 2023-11-29T19:25:36Z

...ges/kbn-check-mappings-update-cli/src/mappings_additions/extract_field_lists_from_plugins.ts

+  const fork = ChildProcess.fork(require.resolve('./extract_field_lists_from_plugins_worker.ts'), {
+    execArgv: ['--require=@kbn/babel-register/install'],
+    cwd: REPO_ROOT,
+    stdio: ['ignore', 'pipe', 'pipe', 'ipc'],
+  });


I was wondering why the other check was using a worker, and decided to go without it, and then I understood: starting the server in this awkward state keeps some open handles, forcing the process to never terminate, so we're effectively forced to fork.

I didn't bother factorizing the fork creating code yet. I will probably if/when we need to add a third check.

I didn't bother factorizing the fork creating code yet. I will probably if/when we need to add a third check.

Good choice, 2 instances of the same/ similar code is fine, a third is a pattern.

starting the server in this awkward state keeps some open handles, forcing the process to never terminate, so we're effectively forced to fork.

We need to add this info somewhere in the package to prevent others from falling into the same trap.

Yeah makes sense, I will add a comment around the place we're invoking the workers

Another consideration was controlling log output more easily, but yeah I remember the non-dying process too

pgayvallet · 2023-11-29T19:26:56Z

packages/kbn-check-mappings-update-cli/src/run_check_mappings_update_cli.ts

+    if (!task || task === 'mapping-addition') {
+      log.info('Running model version mapping addition checks');


Updated the existing package / cli command to run both checks, given it makes more sense to execute everything in a single CI task.

pgayvallet · 2023-11-29T19:32:53Z

...kbn-check-mappings-update-cli/src/mappings_additions/run_versions_mapping_additions_check.ts

+export const runModelVersionMappingAdditionsChecks = async ({
+  fix,
+  verify,
+  log,
+}: {


pseudo-code of the new check:

for each plugin: file_fields = list already checked fields from file mappings_fields = list fields from mappings version_fields = list fields from model versions if file_fields is empty: // new type addition update_file(mappings_fields) return if any field from mappings_fields not in file_fields and not in version_fields => type error if any field from version_fields not in file_fields and not in mappings_fields => type error if any error, fail ci check if no error, update fields in file for all types

pgayvallet · 2023-11-29T19:33:53Z

x-pack/plugins/fleet/server/saved_objects/index.ts

+      '3': {
+        changes: [
+          {
+            type: 'mappings_addition',
+            addedMappings: {
+              secrets: {
+                properties: {
+                  service_token: {
+                    dynamic: false,
+                    properties: {
+                      id: { type: 'keyword' },
+                    },
+                  },
+                },
+              },


@elastic/fleet fixing the mapping that was incorrectly added without a model version in #171875

@pgayvallet I wanted to confirm that the cli check detects problems and ran node scripts/check_mappings_update.js --task mapping-addition locally after removing modelVersion 3 from here.
The check didn't respond with any issues. I'm assuming it should though. Is there some extra flag we need that's not documented well or is something else going on.

FYI: I did bootstrap after removing the model version.

Confirmed, the check only detects when the field is removed from current_fields.json. Is the expectation that mapped fields will never be removed from schema?

Yeah, it's "working as intended": because initial mappings (the ones defined when the type gets introduced) are not in any version, and because there's no way of differentiating those mappings from "new" mappings, we're forced to store the processed fields into the list on disk. So once the field is on the file, if you remove it from the version it was added it, the script will not detect it.

I added an override option (and wired the fix one) in case we need to manual "reset" the file (e.g a PR adding a new type with fields being added in separate commits)

Is the expectation that mapped fields will never be removed from schema?

Correct, for now it is (as we can't remove them from the actual index's mappings atm anyway)

elasticmachine · 2023-11-29T19:34:33Z

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

pgayvallet · 2023-11-30T07:14:18Z

Sorry for the noise everyone :shame:

…apping-additions

kibana-ci · 2023-11-30T12:25:27Z

💚 Build Succeeded

Buildkite Build
Commit: a15fa28

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-saved-objects-base-server-internal`	61	66	+5

Unknown metric groups

API count

id	before	after	diff
`@kbn/core-saved-objects-base-server-internal`	89	98	+9

History

💔 Build #180369 failed 1bd333f
💔 Build #180353 failed 156a996
💚 Build #180236 succeeded 227ed3d
💔 Build #179989 failed ee6630b
💔 Build #179962 failed ced9686

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

add utilities used for the test

62dcf32

pgayvallet added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:Saved Objects labels Nov 28, 2023

pgayvallet and others added 17 commits November 28, 2023 15:10

add test and initial file

c48dbc9

fix references

ae6e356

(will be reverted} add a mapping to the dashboard type to trigger a C…

4f592c1

…I failure

[CI] Auto-commit changed files from 'node scripts/check_mappings_upda…

47ff997

…te --fix'

Revert "[CI] Auto-commit changed files from 'node scripts/check_mappi…

96154ad

…ngs_update --fix'" This reverts commit 47ff997.

Revert "(will be reverted} add a mapping to the dashboard type to tri…

67b7838

…gger a CI failure" This reverts commit 4f592c1.

prepare check mappings package to receive a new check

60f29bd

moving the mapping check to the package

07ed74f

use a worker because we need to

d8f0365

typo

abecb33

(will be reverted) test the CI check by adding new mappings

a50381f

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

4dc71bb

…fix'

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

21d2cae

…-fix'

Revert "(will be reverted) test the CI check by adding new mappings"

4ace973

This reverts commit a50381f.

lint

842525f

Merge remote-tracking branch 'upstream/main' into kbn-172055-detect-m…

ced9686

…apping-additions

fix call

ee6630b

pgayvallet mentioned this pull request Nov 29, 2023

[Fleet] making service_token an output secret #171875

Merged

1 task

pgayvallet added 3 commits November 29, 2023 20:16

fixing first real-time use case !

50f3986

Merge remote-tracking branch 'upstream/main' into kbn-172055-detect-m…

c982dbd

…apping-additions

improve doc

d249421

pgayvallet commented Nov 29, 2023

View reviewed changes

pgayvallet marked this pull request as ready for review November 29, 2023 19:34

pgayvallet requested review from a team as code owners November 29, 2023 19:34

pgayvallet added the release_note:skip Skip the PR/issue when compiling release notes label Nov 29, 2023

pgayvallet requested review from a team as code owners November 30, 2023 07:11

pgayvallet requested review from pzl, parkiino, e40pud and jpdjere November 30, 2023 07:11

pgayvallet force-pushed the kbn-172055-detect-mapping-additions branch from e7ff526 to d781fd9 Compare November 30, 2023 07:12

pgayvallet removed request for a team, pzl, e40pud, jpdjere and parkiino November 30, 2023 07:13

pgayvallet added 4 commits November 30, 2023 08:16

better naming

156a996

use flags

7b11b09

Merge remote-tracking branch 'upstream/main' into kbn-172055-detect-m…

1bd333f

…apping-additions

Merge remote-tracking branch 'upstream/main' into kbn-172055-detect-m…

a15fa28

…apping-additions

pgayvallet merged commit 62d0ce4 into elastic:main Nov 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add CI check to ensure SO mapping addition are done correctly #172056

Add CI check to ensure SO mapping addition are done correctly #172056

pgayvallet commented Nov 28, 2023 •

edited

Loading

pgayvallet left a comment

pgayvallet Nov 29, 2023

pgayvallet Nov 29, 2023

TinaHeiligers Nov 29, 2023

TinaHeiligers Nov 29, 2023

pgayvallet Nov 30, 2023

jloleysens Nov 30, 2023

pgayvallet Nov 29, 2023

pgayvallet Nov 29, 2023

pgayvallet Nov 29, 2023

TinaHeiligers Nov 29, 2023

jloleysens Nov 30, 2023

pgayvallet Nov 30, 2023

elasticmachine commented Nov 29, 2023

pgayvallet commented Nov 30, 2023

kibana-ci commented Nov 30, 2023

API count

		if (!task \|\| task === 'mapping-addition') {
		log.info('Running model version mapping addition checks');

Add CI check to ensure SO mapping addition are done correctly #172056

Add CI check to ensure SO mapping addition are done correctly #172056

Conversation

pgayvallet commented Nov 28, 2023 • edited Loading

Summary

pgayvallet left a comment

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

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

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

Choose a reason for hiding this comment

elasticmachine commented Nov 29, 2023

pgayvallet commented Nov 30, 2023

kibana-ci commented Nov 30, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

History

pgayvallet commented Nov 28, 2023 •

edited

Loading