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

Disallow unknown properties on fields definitions #281

Closed

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Feb 21, 2022

What does this PR do?

Disallow unknown properties on fields definitions.

Why is it important?

Avoid typos as the ones reported in elastic/integrations#2643.

Checklist

Related issues

@jsoriano jsoriano self-assigned this Feb 21, 2022
@jsoriano jsoriano requested a review from a team as a code owner February 21, 2022 10:49
@elasticmachine
Copy link

elasticmachine commented Feb 21, 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-02-21T12:02:41.647+0000

  • Duration: 3 min 15 sec

🤖 GitHub comments

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

  • /test : Re-trigger the build.

Copy link
Contributor

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

I like this idea in general, but I remember we kept it open because we couldn't satisfy all features (multi-fields, example, etc.). Did you check how does this change behaves with existing packages?

@jsoriano jsoriano marked this pull request as draft February 21, 2022 12:02
@jsoriano
Copy link
Member Author

Did you check how does this change behaves with existing packages?

I have checked with some packages now, and I have found that most of the missing properties can be added to the spec.
The only exception would be examples, for that we would need some kind of type: any, or we should change all of them to their string representation.

To add a type, we would need further customization options in the jsonschema library we use. A limitation I have also found in #278.

Changing the examples to their string representation would be an effort in some packages, but wouldn't be a bad idea, these examples are thought for documentation purposes, and it may be good to have control on their format. We actually use strings for everything, except for numbers, so probably the migration effort could be automated.

In any case we could add a semantic validator to check that the examples comply with the field type.

@jsoriano
Copy link
Member Author

I have created #282 to follow on this.

@jsoriano
Copy link
Member Author

The only exception would be examples, for that we would need some kind of type: any, or we should change all of them to their string representation.

If we change all of them to the string representation, we also need to change the ones imported from ECS, there the examples can contain at least numbers.

@mtojek
Copy link
Contributor

mtojek commented Feb 24, 2022

I'm wondering if there aren't any problems with multi fields (see elastic/elastic-package#678).

@jsoriano
Copy link
Member Author

jsoriano commented Feb 24, 2022

I'm wondering if there aren't any problems with multi fields (see elastic/elastic-package#678).

We can validate them (see ee17cfa), this other issue seems to be about importing external fields with multi fields.

@jsoriano
Copy link
Member Author

jsoriano commented Jun 1, 2022

Closing this, after #314 we are in a better situation.

@jsoriano jsoriano closed this Jun 1, 2022
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.

3 participants