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

fix: traverse all the child cmds + duplicates #97

Merged
merged 4 commits into from Jun 30, 2021
Merged

Conversation

ankithans
Copy link
Member

@ankithans ankithans commented Jun 29, 2021

Closes #95

Description

Fixes duplicate errors and traverse all the subcommands

Type of change

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

Checklist

  • Duplication of validation errors
  • Traverse all the subcommands

Screenshots (if appropriate):

@ankithans ankithans changed the title WIP WIP traverse all the child cmds in validator Jun 29, 2021
@ankithans
Copy link
Member Author

ankithans commented Jun 29, 2021

https://github.com/aerogear/charmil/blob/ankithans/issue95/validator/example/cmd.go

cmd
  cmd3
  cmd1
     cmd2

Here only cmd, cmd1, cmd2 are traversed. cmd3 is skipped! Checked with rhoas similar issue is persisting. Fixed the duplicate validation error. Trying to solve this issue

@ankithans ankithans self-assigned this Jun 29, 2021
@ankithans ankithans added enhancement New feature or request bug Something isn't working and removed enhancement New feature or request labels Jun 29, 2021
mustExistErrs := validateMustExist(cmd, config.MustExist, config.Verbose)
info.TotalErrors += len(mustExistErrs)

info.Errors = append(info.Errors, lenErrs...)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should create method that does validate and add errorz for generic rule

continue
}

// recursive call
Copy link
Contributor

@wtrocki wtrocki Jun 29, 2021

Choose a reason for hiding this comment

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

Add verbose logger here


// prints additional info in debug mode
if config.Verbose {
fmt.Printf("commands checked: %d\nchecks failed: %d\n", info.TotalTested, info.TotalErrors)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not verbose. It should be printed normally but to stderr

@ankithans
Copy link
Member Author

This was hard to debug, but got the desired output. Basically, It was a recursion problem 😄. issue #95 resolved

$ go test ./validator/example/
--- FAIL: Test_ExecuteCommand (0.00s)
    cmd_test.go:20: LENGTH_RULE: cmd cmd0: Long length should be at least 100
    cmd_test.go:20: LENGTH_RULE: cmd cmd0: Example length should be at least 100
    cmd_test.go:20: LENGTH_RULE: cmd cmd0 subcmd01 subcmd12: Short length should be at least 15
    cmd_test.go:20: LENGTH_RULE: cmd cmd0 subcmd01 subcmd12: Long length should be at least 100
    cmd_test.go:20: LENGTH_RULE: cmd cmd0 subcmd01 subcmd12: Example length should be at least 100
    cmd_test.go:20: MUST_EXIST_RULE: cmd cmd0 subcmd01 subcmd12: Short must be present
    cmd_test.go:20: MUST_EXIST_RULE: cmd cmd0 subcmd01 subcmd12: Long must be present
    cmd_test.go:20: LENGTH_RULE: cmd cmd0 subcmd01: Short length should be at least 15
    cmd_test.go:20: LENGTH_RULE: cmd cmd0 subcmd01: Long length should be at least 100
    cmd_test.go:20: LENGTH_RULE: cmd cmd0 subcmd01: Example length should be at least 100
    cmd_test.go:20: MUST_EXIST_RULE: cmd cmd0 subcmd01: Short must be present
    cmd_test.go:20: MUST_EXIST_RULE: cmd cmd0 subcmd01: Long must be present
    cmd_test.go:20: LENGTH_RULE: cmd cmd0 subcmd03: Short length should be at least 15
    cmd_test.go:20: LENGTH_RULE: cmd cmd0 subcmd03: Long length should be at least 100
    cmd_test.go:20: LENGTH_RULE: cmd cmd0 subcmd03: Example length should be at least 100
    cmd_test.go:20: MUST_EXIST_RULE: cmd cmd0 subcmd03: Short must be present
    cmd_test.go:20: MUST_EXIST_RULE: cmd cmd0 subcmd03: Long must be present
FAIL
FAIL    github.com/aerogear/charmil/validator/example   0.152s
FAIL

@ankithans ankithans changed the title WIP traverse all the child cmds in validator fix: traverse all the child cmds + duplicates Jun 30, 2021
@ankithans ankithans requested a review from wtrocki June 30, 2021 08:24
@@ -17,21 +17,29 @@ func NewCommand() *cobra.Command {
}

cmd1 := &cobra.Command{
Use: "subcmd",
Use: "subcmd01",
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind to use for loops for creating commands.
It will look better without DRY

}

// prints additional info in debug mode
if config.Verbose {
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets remove verbose check and use sdterr (

func (config *RuleConfig) validate(cmd *cobra.Command, info *validator.StatusLog) {

// Length rule
lenErrs := validateLength(cmd, config.Length, config.Verbose)
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets use for loop for processing rules rather than calling each rule

Copy link
Member Author

@ankithans ankithans Jun 30, 2021

Choose a reason for hiding this comment

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

@wtrocki I have addressed above 2 comments. As all rules gives different function to execute, how can I use them in loop?
like length - validateLength
MustExist - validateMustExist

And rule config also looks like this

type RuleConfig struct {
	Verbose bool
	Length
	MustExist
}

Can you suggest some way?

Copy link
Contributor

@wtrocki wtrocki left a comment

Choose a reason for hiding this comment

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

Lets push this to rhoas but address my comments

@wtrocki wtrocki merged commit 4ba3da6 into main Jun 30, 2021
@wtrocki wtrocki deleted the ankithans/issue95 branch June 30, 2021 10:16
@wtrocki
Copy link
Contributor

wtrocki commented Jun 30, 2021

lets release.

Can you suggest some way?

we should have the same interface and can the same function. Without that there will be no way to add rule to validator. Create issue for this. This two methods should be the same method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate errors on running validator
2 participants