Boeing / config-file-validator

Cross Platform tool to validate configuration files
https://boeing.github.io/config-file-validator/
Apache License 2.0
324 stars 55 forks source link

None of the error returned by reporters are checked #134

Closed ccoVeille closed 5 months ago

ccoVeille commented 6 months ago

In pkg/cli/cli.go there is a section where none of the reporter.Print* method that could return errors are checked

https://github.com/Boeing/config-file-validator/blob/54b6781cb391e40ce70dfbec2974efcedd97f98b/pkg/cli/cli.go#L116-L141

This code requires a bit of refactoring, I think.

The addition of quiet mode (great addition BTW) with #124 made it even more complex to read and maintain.

HakanVardarr commented 6 months ago

I added a new pull request. I did some refactoring on cli.go can you look it?

ccoVeille commented 6 months ago

I added a new pull request. I did some refactoring on cli.go can you look it?

Can you reference the link? Because I looked for it and didn't see it.

HakanVardarr commented 6 months ago

I added a new pull request. I did some refactoring on cli.go can you look it?

Can you reference the link? Because I looked for it and didn't see it.

https://github.com/Boeing/config-file-validator/pull/131#issue-2216294282

ccoVeille commented 6 months ago

I like the refactoring you initiated, but it doesn't address the issue I raised here

Please look at this

https://github.com/Boeing/config-file-validator/blob/e9d7f48043b0c009c30953d5b0be4820d41b776d/pkg/cli/cli.go#L137-L141

Either reporter.PrintSingleGroupJson and reporter.PrintSingleGroupStdout may return an error.

and the error are not tested.

Your code is now simpler, so it's good, but you should consider something like this

       if len(GroupOutput) == 1 && GroupOutput[0] != "" {
        // Check reporter type to determine how to print
        if _, ok := c.Reporter.(reporter.JsonReporter); ok {
            err = reporter.PrintSingleGroupJson(reportGroup.(map[string][]reporter.Report))
        } else {
            err = reporter.PrintSingleGroupStdout(reportGroup.(map[string][]reporter.Report))
        }
    } else if len(GroupOutput) == 2 {
        if _, ok := c.Reporter.(reporter.JsonReporter); ok {
            err = reporter.PrintDoubleGroupJson(reportGroup.(map[string]map[string][]reporter.Report))
        } else {
            err = reporter.PrintDoubleGroupStdout(reportGroup.(map[string]map[string][]reporter.Report))
        }
    } else if len(GroupOutput) == 3 {
        if _, ok := c.Reporter.(reporter.JsonReporter); ok {
            err = reporter.PrintTripleGroupJson(reportGroup.(map[string]map[string]map[string][]reporter.Report))
        } else {
            err = reporter.PrintTripleGroupStdout(reportGroup.(map[string]map[string]map[string][]reporter.Report))
        }
    } else {
        err =  c.Reporter.Print(reports)
    }

    if err != nil {
        return err           
        }

    return nil
HakanVardarr commented 6 months ago

Yeah I will return the error. Thanks for the suggestion.

HakanVardarr commented 6 months ago

Instead of

err = reporter.PrintSingleGroupJson(reportGroup.(map[string][]reporter.Report))

Can't we use

return reporter.PrintSingleGroupJson(reportGroup.(map[string][]reporter.Report))
ccoVeille commented 6 months ago

You refactored it the right way.

I mean the current version is already applying the pattern you talked about.

So you solved the issue with #131 now, which is great.

I will comment the other PR then, because I have a code suggestion