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.

Sign up for GitHub

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

Jump to bottom

fix: skip validation for children #161

Merged
merged 2 commits into from Jul 15, 2021
Merged

fix: skip validation for children #161

merged 2 commits into from Jul 15, 2021

Conversation

ankithans
Copy link
Member

Closes #151

Description

  • fix skip children commands by specifying path

Type of change

  • Bug fix (non-breaking change which fixes an issue)

@ankithans ankithans added the bug Something isn't working label Jul 14, 2021
@ankithans ankithans requested a review from a team July 14, 2021 10:03
@ankithans ankithans self-assigned this Jul 14, 2021
@jjkiely
Copy link
Contributor

jjkiely commented Jul 14, 2021

Have you verified that this fixes the issue? If not can you provide some steps I can follow in order to do so? Thanks!

wtrocki
wtrocki reviewed Jul 14, 2021
View reviewed changes
validator/rules/cmd_test.go
@@ -13,24 +12,19 @@ func Test_ExecuteCommand(t *testing.T) {
// default config can also be overrided
ruleCfg := ValidatorConfig{
ValidatorOptions: ValidatorOptions{
SkipCommands: map[string]bool{"cmd100 cmd0 subcmd01": true},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this removed?

Copy link
Member Author

Choose a reason for hiding this comment

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

no

Copy link
Member Author

Choose a reason for hiding this comment

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

just removed from test

@ankithans
Copy link
Member Author

yep verified myself.
steps are:

  • run make test/validator or go run ./validator/rules
  • change the stuff in validator/rules/cmd_test.go
ValidatorOptions: ValidatorOptions{
	SkipChildren: map[string]bool{
		"root echo": true,
	},
}

this will stop checking root echo and root echo .... children

@wtrocki
Copy link
Contributor

wtrocki commented Jul 14, 2021 •
edited

is this breaking change also removes other flags that we have documented? Best to put more info into PR.

I would like if we can avoid having multiple flags for skipping

Skip for children can be done by

CMD data manipulators *

Where last * means children only.
I'm little bit like broken record but..

lets start with docs and api description

@ankithans
Copy link
Member Author

ankithans commented Jul 14, 2021 •
edited

is this breaking change

not a breaking change actually!

Skip for children can be done by
CMD data manipulators *

got it. this looks cool and is easy to use. Let's do this.

I'm little bit like broken record but..
lets start with docs and api description

you telling about skipping issue or the whole project in general

@wtrocki
Copy link
Contributor

wtrocki commented Jul 14, 2021 •
edited

you telling about skipping issue or the whole project in general.

No. I meant that I will not review PRs without docs or description

@ankithans
Copy link
Member Author

ankithans commented Jul 15, 2021 •
edited

oh okay, if you haven't noticed, this was not a breaking change and docs was already there : https://github.com/aerogear/charmil/blob/validator-skip/docs/src/validator.md#ignore-commands
yeah description was not there, just attached the issue 😅

but now let's do the proposed change and will update the docs.

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

docs: https://github.com/aerogear/charmil/blob/validator-skip/docs/src/validator.md#ignore-commands

@wtrocki
Copy link
Contributor

wtrocki commented Jul 15, 2021 •
edited

Really good. Do we have tests cases matching docs? (outside this PR)

@ankithans
Copy link
Member Author

yep, we have a test using SkipCommands attribute

wtrocki
wtrocki reviewed Jul 15, 2021
View reviewed changes
validator/rules/executor.go Outdated
@@ -70,6 +72,14 @@ func executeRecursive(cmd *cobra.Command, info *validator.StatusLog, ruleConfig

// executeRulesChildren execute rules on children of cmd
func executeRulesChildren(cmd *cobra.Command, info *validator.StatusLog, ruleConfig *RuleConfig, userValidatorConfig *ValidatorConfig) []validator.ValidationError {

// if command's children needs to be ignored
if val, ok := userValidatorConfig.ValidatorOptions.SkipCommands[cmd.CommandPath()+"*"]; ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be helper method

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@wtrocki
Copy link
Contributor

wtrocki commented Jul 15, 2021 •
edited

but now let's do the proposed change and will update the docs.

Docs first please next time.

@ankithans
Copy link
Member Author

Docs first please next time.

yess 💪

@ankithans ankithans merged commit 46c2c11 into main Jul 15, 2021
@ankithans ankithans deleted the validator-skip branch July 15, 2021 15:24
@wtrocki
Copy link
Contributor

wtrocki commented Jul 15, 2021

We can release now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wtrocki wtrocki wtrocki approved these changes

Assignees

@ankithans ankithans

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SkipChildren option not working in validator
3 participants
@ankithans @jjkiely @wtrocki