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

[test] Small enhancement to EcsDynamicTemplatesIT #110740

Merged
merged 8 commits into from
Jul 26, 2024

Conversation

eyalkoren
Copy link
Contributor

@eyalkoren eyalkoren commented Jul 11, 2024

Fixing an error printing bug and making the tests more extensible.

@eyalkoren eyalkoren self-assigned this Jul 11, 2024
@elasticsearchmachine elasticsearchmachine added Team:Data Management Meta label for data/management team v8.16.0 labels Jul 11, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-data-management (Team:Data Management)

@elasticsearchmachine
Copy link
Collaborator

Hi @eyalkoren, I've created a changelog YAML for you.

@ruflin
Copy link
Member

ruflin commented Jul 12, 2024

What is the storage impact of this change? @andrewkroh Has the team by chance look at this?

@ruflin
Copy link
Member

ruflin commented Jul 23, 2024

@eyalkoren Could you give a quick update with the recent discussion on ECS on what the next steps here are? Do we still need this PR?

For anyone looking into this in the future around multiple field types for a single field, some interesting discussions happened here in the past: #53181

@eyalkoren eyalkoren added >test Issues or PRs that are addressing/adding tests and removed >enhancement labels Jul 25, 2024
@eyalkoren
Copy link
Contributor Author

Since the .caseless field mapping changes where revered, this PR is not required anymore.
However, it fixes an error printing bug in the test (which is important because good error message means very easy mappings fix) and it makes the tests more extensible.
It is very small and the it's labelled as test PR now, so a short review would be enough.

@eyalkoren eyalkoren changed the title Adjust ecs@mappings to latest ECS and tests to consider normalizer [test] Small enhancement to EcsDynamicTemplatesIT Jul 25, 2024
@@ -63,7 +63,7 @@ public class EcsDynamicTemplatesIT extends ESRestTestCase {

private static Map<String, Object> ecsDynamicTemplates;
private static Map<String, Map<String, Object>> ecsFlatFieldDefinitions;
private static Map<String, String> ecsFlatMultiFieldDefinitions;
private static Map<String, Map<String, Object>> ecsFlatMultiFieldDefinitions;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Instead of kipping only the subfield's type, keeping all mappings so we can validate any of them

Comment on lines +464 to +466
if (fieldMappings == null) {
fieldMappings = ecsFlatMultiFieldDefinitions.get(fieldName);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The field mappings are either in the top-level mappings or subfields map. We didn't look in the latter before thus error message for subfields was not as descriptive

@eyalkoren eyalkoren merged commit 39cb590 into elastic:main Jul 26, 2024
15 checks passed
@eyalkoren eyalkoren deleted the ecs-tests-fix branch July 26, 2024 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Data Management/Data streams Data streams and their lifecycles Team:Data Management Meta label for data/management team >test Issues or PRs that are addressing/adding tests v8.15.0 v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants