Inappropriate printing to STDOUT

[znewman01]

Generally, in Cosign we try to make STDOUT have the output of the command: the (often machine-readable) result of execution. We put status messages, errors, etc. on STDERR.

This is a common convention for CLI tools and a nice thing to do for scriptability.

However, we're pretty sloppy sometimes.

---

I'm looking mostly at:

• cosign/pkg/cosign/tlog.go

  Line 170 in fa8a799

  fmt.Println("Signature already exists. Displaying proof")

• cosign/cmd/cosign/cli/pivcli/commands.go

  Line 152 in 381ba64

  fmt.Println()

• cosign/cmd/cosign/cli/initialize/init.go

  Line 52 in 381ba64

  fmt.Println("Root status: \n", string(b))

• cosign/cmd/cosign/cli/sign/sign.go

  Line 250 in d720f04

  fmt.Printf("Certificate wrote in the file %s\n", outputCertificate)

But even some of the ones that are actually correct just work by spitting out to STDOUT in the middle of command execution:

• cosign/cmd/cosign/cli/pivcli/commands.go

  Line 265 in 381ba64

  fmt.Println(string(pemBytes))

• cosign/cmd/cosign/cli/tree.go

  Line 152 in d720f04

  fmt.Printf("%s 🍒 %s\n", sym, digest)

---

My proposal is the following:

1. Have the CLI top-level entry point print the output of the command. That is, something like SaveCmd would be implemented as a function with a return value, and then we'd hook into Cobra to print the output (if we need to stream output, we could use channels or something).
2. Ban (using Forbidigo) any calls to fmt.Print{f,ln} in the Cosign codebase.
3. Implement a "Cosign output" package. Instead of fmt.Fprintln(os.Stderr, "warning: foo") directly, Cosign code would call Warn(). Same for any other common output patterns. (That is, calling code can assign semantics to its output which helps make sure it goes to the right place).
4. Ban (most) other fmt.* calls (we can keep fmt.Sprint*).

This has a number of benefits:

• Supporting multiple formats easily and consistently (you could pass --format=jsonl or --format=csv) without requiring support from every command
• Testability: parsing STDOUT is hard. Right now that's part of the reason that the cmd/ directory is under-tested.
• Enforce consistent output: of course, this fixes the STDOUT/STDERR thing. But it also starts to unify the output across the whole tool. If we wanted to start putting all STDERR output in red, we could do that in one place easily. If we want to "tee" all output to a log file, we can do that.

[eddiezane]

The way we handle this in kubectl and gitsign is by encapsulating the streams which has the added bonus of being really easy to mock in tests.

[znewman01]

The way we handle this in kubectl and gitsign is by encapsulating the streams which has the added bonus of being really easy to mock in tests.

Yeah I think this is also a good idea and can be implemented at the same time we do (3) above.