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

feat: add ignore command option #144

Merged
merged 3 commits into from Jul 7, 2021
Merged

feat: add ignore command option #144

merged 3 commits into from Jul 7, 2021

Conversation

ankithans
Copy link
Member

Closes #86

Description

IgnoreCommands map[string]bool option in ValidatorOptions, for ignoring validation

Type of change

  • New feature (non-breaking change which adds functionality)
  • Documentation change

Checklist

  • Documentation added for the feature
  • CI and all relevant tests are passing
  • Code Review completed
  • Verified independently by reviewer

@ankithans ankithans added the enhancement New feature or request label Jul 6, 2021
@ankithans ankithans self-assigned this Jul 6, 2021
@ankithans
Copy link
Member Author

@aerogear/charmil-core

@wtrocki wtrocki requested a review from a team July 6, 2021 08:52
func validate(cmd *cobra.Command, info *validator.StatusLog, ruleConfig *RuleConfig, userValidatorConfig *ValidatorConfig) {

// if command needs to be ignored
if val, ok := userValidatorConfig.ValidatorOptions.IgnoreCommands[cmd.Use]; ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

How this will work with nested commands? Do we use . to map the nested commands.

Example:

kafka.create 
registry.create

Copy link
Member Author

Choose a reason for hiding this comment

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

it captures Use of the command, so

kafka.create 
registry.create

all create subcommands will be ignored. Let's fix this!

Copy link
Contributor

@wtrocki wtrocki Jul 6, 2021

Choose a reason for hiding this comment

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

However, would that be hard? Can we simply create method that returns true or false for the current command and rule rule?

EDIT:

Ignore me that will be bad idea.

I think we might need something as follows:

  • Ignore entire chain of commands (when you do dev etc.) and still want tests to pass
  • Ignore single command in nested scenario for all rules
  • Ignore single command in nested scenario for some rule.

Instead of impleentatation I would love to see config you will come up with so we can discuss it.
Remeber - we focus on API and docs first :D

Copy link
Member Author

@ankithans ankithans Jul 6, 2021

Choose a reason for hiding this comment

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

for nested/all commands, best way is to use CommandPath() provided by cobra.
user will specify : {"kafka create": true, "echo": true}

This config is coming to my mind atm -

type ValidatorOptions struct {
	Verbose      bool            
	SkipChildren map[string]bool 
	SkipCommands map[string]bool 
}

RuleOptions to be present in each rule, for controlling rule specific properties/options

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

eg -

type Length struct {
	RuleOptions validator.RuleOptions
	Limits      map[string]Limit
}

Copy link
Contributor

@wtrocki wtrocki Jul 6, 2021

Choose a reason for hiding this comment

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

type Length struct {
	RuleOptions validator.RuleOptions

Could that be part of the interface so we can still have generic processing and enforce options for each rule

SkipChildren map[string]bool 

Not needed/confusing. If you specify command path that is groping other commands all grouped ones will be disabled.

 map[string]bool 

Not sure if that would give us enough to control command (path to command), rule, level - Error/Ignore (could be warn in the future)

Copy link
Member Author

Choose a reason for hiding this comment

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

RuleOptions validator.RuleOptions
Could that be part of the interface so we can still have generic processing and enforce options for each rule

Rules interface?

SkipChildren map[string]bool
Not needed/confusing. If you specify command path that is grouping other commands all grouped ones will be disabled.

can you please provide an example for understanding

map[string]bool
Not sure if that would give us enough to control command (path to command), rule, level - Error/Ignore (could be warn in the future)

do we need all this details, when we ignore some command?

Copy link
Contributor

Choose a reason for hiding this comment

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

do we need all this details, when we ignore some command?

Agreed. No need

@wtrocki
Copy link
Contributor

wtrocki commented Jul 6, 2021

I'm good to merge this :) after we add documentation

@ankithans
Copy link
Member Author

I'm good to merge this :) after we add documentation

Yes, but shouldn't these be included as well?

Ignore entire chain of commands (when you do dev etc.) and still want tests to pass
Ignore single command in nested scenario for all rules
Ignore single command in nested scenario for some rule.

@wtrocki
Copy link
Contributor

wtrocki commented Jul 6, 2021

You can try adding some quick wins :D

@ankithans ankithans requested a review from wtrocki July 7, 2021 07:01
@ankithans
Copy link
Member Author

Working

Ignore Commands

Sometimes during development, you want to pass the tests for certain commands, but at the same time use Validator for tests. Validation can be skipped/ignored for the commands, mentioned in the validator configuration.
To ignore the commands you need to specify the path of the command in validator configuration.

  1. Skip single command
ValidatorOptions: rules.ValidatorOptions{
	SkipCommands: map[string]bool{"cmd100 cmd0 subcmd01": true},
},
  1. Skip the command including all children
ValidatorOptions: rules.ValidatorOptions{
	SkipChildren: map[string]bool{"cmd100": true},
},
  1. Skip the command for specific rule
Length: rules.Length{
	RuleOptions: validator.RuleOptions{
		SkipCommands: map[string]bool{"cmd100": true},
	},
	Limits: map[string]rules.Limit{
		"Use": {Min: 1},
	},
},

Length: rules.Length{
RuleOptions: validator.RuleOptions{
SkipCommands: map[string]bool{"mycli actions create": true},
Copy link
Contributor

Choose a reason for hiding this comment

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

@ankithans is that ok?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes.. this looks good!

Copy link
Contributor

Choose a reason for hiding this comment

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

example had only create command so I wasn't sure if you are using cobra paths for this

@wtrocki wtrocki merged commit 218c972 into main Jul 7, 2021
@wtrocki wtrocki deleted the ankithans/issue86 branch July 7, 2021 08:55
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
None yet
Development

Successfully merging this pull request may close these issues.

Config with array of commands during validation
2 participants