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

Stricter validation of field definitions #420

Merged
merged 3 commits into from
Sep 20, 2022

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Sep 15, 2022

What does this PR do?

Stricter validation of field definitions.

Why is it important?

To reduce the risk of typos in field definitions that can lead to unexpected mappings.

Checklist

  • I have added test packages to test/packages that prove my change is effective.
  • I have added an entry in spec/changelog.yml.
  • Test these changes with current integrations.

Related issues

@jsoriano jsoriano self-assigned this Sep 15, 2022
@elasticmachine
Copy link

elasticmachine commented Sep 15, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-09-15T15:16:04.342+0000

  • Duration: 7 min 4 sec

Test stats 🧪

Test Results
Failed 0
Passed 632
Skipped 0
Total 632

🤖 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
Copy link

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (7/7) 💚
Files 68.0% (17/25) 👍
Classes 76.471% (26/34) 👍
Methods 56.075% (60/107) 👍
Lines 42.184% (537/1273) 👍
Conditionals 100.0% (0/0) 💚

@jsoriano jsoriano changed the title Fields strict validation Stricter validation of field definitions Sep 15, 2022
@jsoriano
Copy link
Member Author

Tested with integrations and some adjustments done. I think that the number of changes required on existing integrations now are reasonable.

@jsoriano jsoriano marked this pull request as ready for review September 15, 2022 15:16
@jsoriano jsoriano requested a review from a team as a code owner September 15, 2022 15:16
additionalProperties: true
additionalProperties: false
patternProperties:
# Soft validation on some properties that can be present but not validated or used yet.
Copy link
Member Author

@jsoriano jsoriano Sep 15, 2022

Choose a reason for hiding this comment

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

The main reason I am leaving it here is to reduce the number of changes that package developers should make to migrate to v2, and to avoid conflicts with fields copied from ECS or Beats. But these fields are not really used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, if it is not required too many changed done by developer to migrate, it will be easier that packages get updated to 2.0.0 👍
Those could be removed in a following major if it is required.

Copy link
Contributor

@mrodm mrodm left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Change Proposal] Stricter validation on fields properties
3 participants