Add ECS mappings during test runner executions by mrodm · Pull Request #1090 · elastic/elastic-package

mrodm · 2023-01-10T10:56:27Z

Follow-up of #1073

This PR adds all the ECS field definitions available in test executions to check whether or not a field in the sample event is defined. None of these field definitions are added in the built package.

The ECS version used for that regard is the version set at _dev/build/build.yml in the package itself.

ECS definitions are used instead of trying to convert the ecs_mappings.json to fields in elastic-package mainly because:

those dynamic templates in ecs_mappings.json use match, patch_unmatch, that do not have a relation 1 to 1 with the field definitions as managed in elastic-package.
ECS schema explicitly defines all the field definitions.

These mappings are going to be used just when that flag is enabled.

A new test package has been added with the import_mappings flag is enabled, and for that it requires package-spec 2.3.0 to be updated in elastic-package.

This PR is based on the branch of #1073. To review just the commits from this PR, it should be checked from commit 26c8cfa (inclusive).

Requisites:

package-spec 2.3.0 published and updated in elastic-package (required by the test package)

…t exist

Review errors raised and their messages

…ppings

In kibana versions < 8.6.0 the dot in the names caused that packages were not installed. It was interpreted that the string after the dot was an inner element of the map, causing validation errors.

This reverts commit a46de7b.

elasticmachine · 2023-01-10T11:30:43Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-01-13T17:00:59.054+0000
Duration: 33 min 11 sec

Test stats 🧪

Test	Results
Failed	0
Passed	878
Skipped	0
Total	878

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

elasticmachine · 2023-01-10T11:30:52Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`35/35`)	💚
Files	65.909% (`87/132`)	👍
Classes	61.376% (`116/189`)	👍 0.205
Methods	48.642% (`394/810`)	👍 1.148
Lines	31.707% (`3555/11212`)	👍 0.848
Conditionals	100.0% (`0/0`)	💚

mrodm · 2023-01-10T11:31:40Z

/test

mrodm · 2023-01-10T12:14:58Z

go.mod

 	sigs.k8s.io/yaml v1.3.0 // indirect
 )
+
+replace github.com/elastic/package-spec/v2 => github.com/mrodm/package-spec/v2 v2.0.0-20221220145202-70d4563fbf33


To be removed before merging

mrodm · 2023-01-10T12:15:37Z

internal/builder/packages.go

 	}

-	logger.Debug("Add dynamic mappings if needed")
+	logger.Debug("Add dynamic mappings if needed (technical preview)")


Would it be interesting to add this ? Maybe changing to INFO instead ?

Sounds good to add this. If changed to info it should probably appear only if the feature is used.

Ok! I'll move this message as an INFO to be shown just when the feature is enabled/used.

mrodm · 2023-01-10T15:04:50Z

test/packages/other/imported_mappings_tests/manifest.yml

@@ -0,0 +1,20 @@
+format_version: 2.3.0


This makes necessary package-spec 2.3.0 to be published.

jsoriano

Nice to see it working, added a possible improvement.

jsoriano · 2023-01-11T08:19:01Z

internal/fields/validate.go

+		ecsSchema, err := v.FieldDependencyManager.ImportAllFields(defaultExternal)
+		if err != nil {
+			return err
+		}


This will import ECS for each field, right?
Would it be possible to combine the v.Schema and the result of v.FieldDependencyManager.ImportAllFields(defaultExternal) in CreateValidatorForDirectory(...)? So we would only import ECS once and this logic here wouldn't be needed.

I was worried in case all these ECS field definitions were added also for instance in the exported field documentation.

I've been checking again this, and when it is used to get the exported fields:

validator, err := fields.CreateValidatorForDirectory(fieldsParentDir) if err != nil { return "", errors.Wrapf(err, "can't create fields validator instance (path: %s)", fieldsParentDir) }

no spec version is used as a ValidatorOption parameter, so the default value (0.0.0) is taken. That makes sure that all these fields are not used at all in this stage since it must be at least 2.3.0. In any case, I've added fields.WithDisabledImportAllECSSChema() to ensure that feature is not used there.

I'll do the changes to move this logic directly in CreateValidatorForDirectory()

Thanks!

mrodm · 2023-01-11T15:27:03Z

/test

jsoriano

I am wondering now if we are correctly deciding when to import the schema for tests. It should be only imported for packages that include the ECS dynamic mappings.

internal/fields/validate.go

internal/docs/exported_fields.go

This new datastream allows us to test the case where there is a field that is defined in both ECS and package fields.

jsoriano · 2023-01-12T14:51:04Z

internal/fields/validate_test.go

 }

+func TestValidate_WithEnabledImportAllECSSchema(t *testing.T) {
+	finder := packageRootTestFinder{"../../test/packages/other/imported_mappings_tests"}


internal/docs/exported_fields.go

mrodm · 2023-01-13T11:52:19Z

/test

This reverts commit 26c8cfa.

mrodm · 2023-01-13T16:15:56Z

/test

mrodm · 2023-01-13T17:00:46Z

/test

mrodm added 25 commits December 15, 2022 17:44

Add dynamic mappings and properties - pending to create if they do no…

6e51991

…t exist

Write files in destinationDir

89e943c

Create nodes if they do not exist

9b7624b

Test new version package-spec

f51aef7

Check wether or not import setting is true in build manifest

c663afe

Added list of mappings added into _meta field

3c78387

Add tests for methods adding ecs mappings

001a312

Add license header

ddea438

Update field used to import mappings

e85ada3

Rename dynamic template names to include a common prefix

44763fc

Review errors raised and their messages

Remove properties

311ee70

Remove the addition of _meta fields

f6fcff5

Add debug message before validation

17d6ac3

Update debug messages

31fa9a2

Add method to check if mappings should be imported

aaa83e2

Merge remote-tracking branch 'upstream/main' into add_temporal_ecs_ma…

57da695

…ppings

Use semver v3

f21c828

Remove package-spec replace from go.mod

a46de7b

Replace dot by a dash in the imported template names

8290c65

In kibana versions < 8.6.0 the dot in the names caused that packages were not installed. It was interpreted that the string after the dot was an inner element of the map, causing validation errors.

Revert "Remove package-spec replace from go.mod"

26c8cfa

This reverts commit a46de7b.

Add example test package importing packages

43376cf

Add method to import all ECS schema

07184ab

Check ECS schema for field validation when needed

fde6430

Add technical preview message

a281b9c

Just allow import ECS mappings in tests if spec >= 2.3.0

4daa034

mrodm self-assigned this Jan 10, 2023

mrodm mentioned this pull request Jan 10, 2023

Temporarily import hard-coded list of common fields #1018

Closed

3 tasks

mrodm commented Jan 10, 2023

View reviewed changes

Add test for disabled import all ECS definitions

7c6685c

mrodm marked this pull request as ready for review January 10, 2023 17:22

mrodm requested a review from a team January 10, 2023 17:23

jsoriano reviewed Jan 11, 2023

View reviewed changes

mrodm added 2 commits January 11, 2023 13:29

Add ECS definitions to the schema in CreateValidatorForDirectory

589136a

Show technical preview message if feature is used

cbfa849

Ensure that for exported fields imported ECS defintions is disabled

7ca93b3

mrodm requested a review from jsoriano January 11, 2023 17:19

jsoriano reviewed Jan 12, 2023

View reviewed changes

internal/fields/validate.go Outdated Show resolved Hide resolved

internal/fields/validate.go Outdated Show resolved Hide resolved

internal/fields/validate.go Outdated Show resolved Hide resolved

internal/docs/exported_fields.go Outdated Show resolved Hide resolved

mrodm added 5 commits January 12, 2023 10:41

Rename variable about importing ECS schema

e4cd4a1

Add new datastream in test package

8b27f4b

This new datastream allows us to test the case where there is a field that is defined in both ECS and package fields.

Review enable import ECS definitions parameter

9ec2342

Enable import ECS schema in test runners

4fbe783

Use an interface to get package root

d44654c

mrodm requested a review from jsoriano January 12, 2023 13:21

jsoriano approved these changes Jan 13, 2023

View reviewed changes

Remove redundant ValidatorOption from exported_fields

c62cf03

mrodm added 2 commits January 13, 2023 13:35

Revert "Revert "Remove package-spec replace from go.mod""

5226564

This reverts commit 26c8cfa.

Merge remote upstream/main into add_all_ecs_mappings_in_test

293861f

mrodm merged commit 2794a56 into elastic:main Jan 13, 2023

mrodm deleted the add_all_ecs_mappings_in_test branch January 13, 2023 17:42

Conversation

mrodm commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Uh oh!

elasticmachine commented Jan 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🌐 Coverage report

Uh oh!

mrodm commented Jan 10, 2023

Uh oh!

mrodm Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

mrodm Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

jsoriano Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

mrodm Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

mrodm Jan 10, 2023

Choose a reason for hiding this comment

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

jsoriano Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

mrodm Jan 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrodm commented Jan 11, 2023

Uh oh!

jsoriano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jsoriano Jan 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrodm commented Jan 13, 2023

Uh oh!

mrodm commented Jan 13, 2023

Uh oh!

mrodm commented Jan 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mrodm commented Jan 10, 2023 •

edited

Loading

elasticmachine commented Jan 10, 2023 •

edited

Loading

elasticmachine commented Jan 10, 2023 •

edited

Loading

mrodm Jan 11, 2023 •

edited

Loading