Set non-zero exit code for invalid manifests

[MaGaroo]

Hi!
This PR is supposed to resolve #61

I also refactored a small part of the code in order to remove redundancy in the print section.
I'd be glad if you could review these changes.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: MaGaroo
Once this PR has been reviewed and has the lgtm label, please assign richabanker for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[apelisse]

Does this mean that we're not going to print the errors anymore?

[MaGaroo]

Cobra prints the error itself. I think we should either print the error manually and set SilenceError attribute in cobra.Command or not print the error explicitly and let cobra print it for us.

[apelisse]

json.Marshal is stable since it sorts the keys, so this is not going to change the behavior, but printHumanErrors is now going to show files in a random order. Not sure how much it matters but I wanted to point that out.

[MaGaroo]

Currently not sure about the best approach, but wanted to note another change in behavior of human formatter.
Previously, if a file had occurred twice in the args, human output printed it twice too. Now, it only prints its errors once.

[apelisse]

I'm not super excited about that.

Something you could do is:

1. Give FilesErrors a "Append" function that supports self being nil
2. return the instance here without checking for nil, nor creating a dummy error

So probably something like this:

type Error struct {
    path string
    err error
}

type FilesErrors []Error

func (e *FilesError) Append(path string, err error) *FilesError {
    if err == nil {
        return e
    } 
    return append(e, Error{path: path, err: err})
}

func (e *FilesError) Error(format string) string {
    // print according to format, can call separate functions if needed.
}

... // In Run()
var errs *FileErrors // Get a nil pointer to errors
for _, path := range files {
    errs = errs.Append(path, ValidateFile(path, factory))
}
...
return errs // Will be null if we've never added any error.

... // in main()
if err := rootCmd.Execute(); err != nil {
    fmt.Fprintln(os.Stderr, err.Error(rootCmd.outputFormat))
    os.Exit(1)
}

I think this has a lot less conditions to keep track of.

[MaGaroo]

I generally agree, but there are some concerns. I start with two of them which are the most important ones:

First, each file can contain multiple documents.¹ ValidateFile function generates an error (possibly nil) for each document and returns a list of errors. ² So, it's important to keep track of nil errors as json formatter needs it.

Second, in order for the FilesErrors type to be considered a golang error, we have to implement Error method without any arguments. So, what's your opinion about declaring FilesErrors as a struct with errors and outputFormat fields, and using outputFormat field to implement the logic of Error() method?

Footnotes

1. It runs ValidateDocument over all documents of the file, which returns exactly one error per document. So, ValidateFile returns a slice containing those errors, i'th error for the i'th document. ↩

2. I think the human output is a little ambiguous when a file contains multiple documents. For example, if the first and third documents in a file have an error and the second document is sane, the human formatter prints the first and third errors and prints nothing about the sanity of the second document. However, this issue is out of scope and I intended to address it in another issue/pr. ↩

[apelisse]

Sorry for the delay, code freeze in k/k is approaching and I lost track of that.

Yeah, I made the invalid assumption that we weren't printing anything if there was no error.
Considering that, I don't see how we can return a relevant error out of the Run method, maybe it's fine to just os.Exit directly from it, and always return nil when no errors occur?

[alexzielenski]

I'd prefer for the cobra command to return an errors.Join of a slice of the Error type @apelisse posted above, then in main we can exit if the execution returns non-nil error.

[k8s-ci-robot]

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[alexzielenski]

I've opened up #63 since this PR has gotten stale and this change is super high priority.

after that merges you may like it keep working on your refactoring, or close this PR

[MaGaroo]

Sorry for the delay, for some reason I didn't receive any notification about this PR.
So, if it is completed, should we close this PR, or is there anything I can do?

[apelisse]

I think we can close, thank you!