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

Configurable TopologyValidator plugin mechanism #249

Closed

Conversation

LeonardoBonacci
Copy link
Contributor

@LeonardoBonacci LeonardoBonacci commented Mar 31, 2021

By solving this issue, this PR also introduces some configurable flexibility in the validator 'framework'. It allows a potential String parameter to be inserted in the constructor by naming convention (# is used as a separator in this example). It can then be used in the corresponding TopologyValidatorImpl at will.

It's backwards compatible.

Still work in progress. Feedback is welcome and community acceptance is required before proceeding.

  • Please check if the PR fulfills these requirements
  • The commit messages are descriptive
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • An issue has been created for the pull requests. Some issues might require previous discussion.

@purbon purbon self-requested a review April 1, 2021 08:10
@LeonardoBonacci
Copy link
Contributor Author

Great ideas, thanks for that!
For the sake of communication I have implemented and committed your suggestions.

The TopologyValidator change is reverted and the TopicNameRegexValidation takes its argument(s) from the config, like this:
topology.validations.0=com.purbon.kafka.topology.validation.topology.TopicNameRegexValidation
topology.validations.regexp=foo.bar.[a-zA-Z]+.[a-zA-Z]+

One thing to notice though is that you suggested to add the regex in the config topology.validations.regexp.0
With the implemented decoupling and the possibility to chain Validators it is not certain to be validation 0 (it might as well be 1, 2, etc.). Therefore I have used (and this is up to the developer of each Validator), a generic config path without the numerical component topology.validations.regexp

No need to take the PR yet - if everyone agrees I will make a clean commit without the intermediate commits of this branch.

@purbon
Copy link
Collaborator

purbon commented Apr 6, 2021

Hi, quick question, could it happen for a user to have the need of more than one regexp to validate topics? a fair answer would it be that not, so we only have one regular expression and one topic name validator. What do you think?

If that would be the case, do you think we could add a validation for this config, a check like if you tried to have more than one TopicNameRegexValidation, raise an exception. Does this make sense?

@purbon
Copy link
Collaborator

purbon commented Apr 6, 2021

On the other side, before we merge this in, would you please be so nice to add a few notes (does not need to be a bible 😄 ) in the documentation about it? thanks a lot.

@purbon
Copy link
Collaborator

purbon commented Apr 6, 2021

I really like where this is going! thanks a lot for your help here @LeonardoBonacci !! really appreciated

@LeonardoBonacci
Copy link
Contributor Author

PR in progress

@purbon
Copy link
Collaborator

purbon commented Apr 16, 2021

Hi @LeonardoBonacci, do you mind adding test for the validation?

@purbon
Copy link
Collaborator

purbon commented May 1, 2021

Hi @LeonardoBonacci, if you don't mind I would import these changes within #274, add test and add it to master! thanks a lot for your contribution.

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.

2 participants