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

Feature to disable a validation rule #197

Closed
ankithans opened this issue Aug 2, 2021 · 4 comments · Fixed by #202
Closed

Feature to disable a validation rule #197

ankithans opened this issue Aug 2, 2021 · 4 comments · Fixed by #202
Assignees
Labels
enhancement New feature or request
Projects

Comments

@ankithans
Copy link
Member

ankithans commented Aug 2, 2021

User should be able to disable a rule, by passing a boolean "enable". Default set to true.

Current

type RuleOptions struct {
	Verbose      bool
	SkipCommands map[string]bool
}

Proposed

type RuleOptions struct {
	Disable       bool
	Verbose      bool
	SkipCommands map[string]bool
}
@ankithans ankithans added the enhancement New feature or request label Aug 2, 2021
@ankithans ankithans added this to To do in Charmil WIP via automation Aug 3, 2021
@ankithans
Copy link
Member Author

ankithans commented Aug 3, 2021

I found two approaches to do this -

  • Stop rule from executing from RuleExecutor(means donot add the particular rule into rules array to execute)
  • add rules to executor; execute the rule and inside each rule check if it is disabled - return empty errors.

I think first one is good? Need suggestions to proceed @aerogear/charmil

@jackdelahunt
Copy link
Contributor

So what your saying in option 1 is to check before we add all of the rules here? If this is where you are thinking about I would maybe suggest option 2 as option 1 feels like it is hidden away and is not an obvious place where we do that check.

Option 2 which should just need an edit to this for loop here I think so adding the check here would just make more sense.... for me atleast.

@wtrocki
Copy link
Contributor

wtrocki commented Aug 4, 2021

option 1 for me. but both are acceptable

@ankithans
Copy link
Member Author

ankithans commented Aug 4, 2021

Option 2 which should just need an edit to this for loop here I think so adding the check here would just make more sense.... for me atleast.

here it cannot be done, as rule interface only has validate method, no access to RuleOptions. Let's go with option 1

and in 2nd opt, it need to be done here in each rule - https://github.com/aerogear/charmil/blob/main/validator/rules/length.go#L43

Charmil WIP automation moved this from To do to Done Aug 4, 2021
@ankithans ankithans self-assigned this Aug 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Charmil WIP
  
Done
Development

Successfully merging a pull request may close this issue.

3 participants