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

Add recursive validation to config #6576

Closed
wants to merge 1 commit into from

Conversation

mattsains
Copy link
Contributor

Closes #4584

Description:
This PR, which is heavily based on @FreakyNobleGas 's work, allows for children of config structs to define their own Validate method. All nodes' Validate methods will be checked instead of just the root of the config element.

If a validation error is found within the config tree, it will be surfaced with a message like this:

extension "ExtensionName" has invalid configuration in "ChildList[1].AnElementWithValidator": The error message returned by the validator within AnElementWithValidator

Link to tracking Issue:
#4584

Testing:
I added two new tests to check that children of config are validated. I ran these tests through make all in the root of the project.

I think the testing I've done is a bit weak just because of my unfamiliarity with the project, so I'm happy to do more testing if needed.

Open question: If a config struct has a Validate method implemented, but that validation succeeds, should we continue checking inside the children of that object, or should we consider the entire object "checked" by that implementation? Right now, I've chosen the former option.

@mattsains mattsains requested review from a team and tigrannajaryan November 18, 2022 00:20
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 18, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: mattsains / name: Matthew Sainsbury (cacd895)

@bogdandrutu
Copy link
Member

Please take a look at #6545, where we want to add the validation.

@mattsains
Copy link
Contributor Author

Damn, that sucks, looks like my PR is useless then (I'll close it). @bogdandrutu is any help needed on the other PR? Otherwise I'll just find something else to do

@mattsains mattsains closed this Nov 18, 2022
@bogdandrutu bogdandrutu reopened this Nov 22, 2022
@mattsains mattsains deleted the 4584 branch December 14, 2022 17:43
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.

exporterhelper should reject a queue size of 0
2 participants