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

Consistently use STDERR for output. #647

Merged
merged 1 commit into from
Sep 12, 2021
Merged

Conversation

mattmoor
Copy link
Member

This is basically these commands (I also renamed a variable in the output-capture code):

sed -i 's@fmt.Printf(@fmt.Fprintf(os.Stderr, @g' $(git grep fmt.Printf | cut -d':' -f 1 | uniq)
sed -i 's@fmt.Println(@fmt.Fprintln(os.Stderr, @g' $(git grep fmt.Println | cut -d':' -f 1 | uniq)
sed -i 's/os.Stdout/os.Stderr/g' $(git grep os.Stdout | cut -d':' -f 1)

(some context around motivation)

I have been experimenting with adding inline keyless verification in downstream tooling with:

	if cli.EnableExperimental() {
		if err := cli.Verify().Exec(cmd.Context(), []string{opts.ref.String()}); err != nil {
			return err
		}
	}

(and similar for keyless signing, but that's more verbose currently, so omitted)

However, some of these tools reserve STDOUT to enable command composition, e.g.

kn service create foo --image=$(ko publish ./cmd/bar)

So if (in this illustrative example) we wanted ko to automatically verify base images when COSIGN_EXPERIMENTAL=true then we would need all of the output from cli.Verify().Exec(...) to either go to STDERR, or surface some capacity for redirecting output (similar to cobra's cmd.Set{Out,Err} methods).


There is some existing logic to redirect STDOUT to a file, but I couldn't really discern a pattern for what goes to STDOUT vs. STDERR. The two general motivations for this (I'd assume) would be:

  1. Quiet and/or capture the full command output for publishing (in which case, it was missing all the STDERR today!)
  2. To capture schematized data to a file for processing by downstream tools (e.g. the way ko publish emits a digest to STDOUT, and nothing else; but there is no obvious schema to any of the paths I looked at...?)

So I'm assuming that the motivation is 1., and "fixing" it to capture STDERR here, but it would be good to get some eyeballs on this that are more familiar with the motivation for capturing output to make sure this isn't somehow breaking (let's see what the tests say)!

Signed-off-by: Matt Moore mattomata@gmail.com

This is basically these commands:
```shell
sed -i 's@fmt.Printf(@fmt.Fprintf(os.Stderr, @g' $(git grep fmt.Printf | cut -d':' -f 1 | uniq)
sed -i 's@fmt.Println(@fmt.Fprintln(os.Stderr, @g' $(git grep fmt.Println | cut -d':' -f 1 | uniq)
sed -i 's/os.Stdout/os.Stderr/g' $(git grep os.Stdout | cut -d':' -f 1)
```

Signed-off-by: Matt Moore <mattomata@gmail.com>
@dlorenc dlorenc merged commit fefa881 into sigstore:main Sep 12, 2021
@dlorenc
Copy link
Member

dlorenc commented Sep 12, 2021

Whoops, this did hit an issue with sign-blob signatures going to stdout. This only got hit in our post-merge tests :(

dlorenc added a commit that referenced this pull request Sep 12, 2021
dlorenc added a commit that referenced this pull request Sep 12, 2021
This reverts commit fefa881.

Signed-off-by: Dan Lorenc <lorenc.d@gmail.com>
@cpanato cpanato added this to the v1.2.0 milestone Sep 12, 2021
dlorenc pushed a commit that referenced this pull request Sep 12, 2021
This reverts commit fefa881.

Signed-off-by: Dan Lorenc <lorenc.d@gmail.com>
@mattmoor mattmoor deleted the use-stderr branch September 12, 2021 15:27
@mattmoor
Copy link
Member Author

@dlorenc Do you have a link to which test? I'm wondering if we can add presubmit coverage, and I'd be curious what things care about STDOUT vs. STDERR, to help rationalize things and build my mental model for it.

@dlorenc
Copy link
Member

dlorenc commented Sep 12, 2021

You should be able to see this: https://github.com/sigstore/cosign/runs/3579958722?check_suite_focus=true

It looks like we expect the signature over STDOUT in sign-blob.

I think we also expect the verified payloads in verify (the JSON simple singing thing).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants