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

scripts: Optionally disallow additionalProperties at the root #86

Merged

Conversation

maxbrunet
Copy link
Contributor

@maxbrunet maxbrunet commented Dec 7, 2021

It can useful to add additionalProperties to the root of the schema to detect extraneous keys.

(the new tests are very nice!)

This also adds a new line at the end of the schema files.

@maxbrunet maxbrunet force-pushed the hotfix/scripts/root-additionalProperties branch from b50eefe to 5ac3c05 Compare December 7, 2021 02:44
@maxbrunet maxbrunet changed the title scripts: Fix missing root additionalProperties scripts: Optionally disallow additionalProperties at the root Dec 11, 2021
@maxbrunet
Copy link
Contributor Author

I have made the change to be opt-in only as it is not absolutely required for a CRD to have a complete schema for build-in fields, since these are implicitly validated. See strimzi/strimzi-kafka-operator#5997 for example. Although, most have them.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@maxbrunet maxbrunet force-pushed the hotfix/scripts/root-additionalProperties branch from 1c9332a to 67a73a9 Compare December 11, 2021 19:40
@yannh
Copy link
Owner

yannh commented Dec 12, 2021

Hi Max, thanks! Could you add a couple of tests to demonstrate the new behaviour of additional_properties? 🙏 Thanks 🙇

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@maxbrunet maxbrunet force-pushed the hotfix/scripts/root-additionalProperties branch from 0ed5cac to 30528c5 Compare December 12, 2021 19:44
@maxbrunet
Copy link
Contributor Author

Done! I have added tests for both FILENAME_FORMAT and DENY_ROOT_ADDITIONAL_PROPERTIES

@yannh
Copy link
Owner

yannh commented Dec 12, 2021

Sweet thanks! A unit test would be nice some day but this is good already 💯

@yannh yannh merged commit c1b3e93 into yannh:master Dec 12, 2021
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.

None yet

2 participants