Skip to content

Simplify exposed output package methods #402

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

Merged
merged 1 commit into from
Sep 22, 2020

Conversation

jpreese
Copy link
Member

@jpreese jpreese commented Sep 18, 2020

Continuing to work towards more stable and usable package APIs for a conftest v1 release, this PR reworks the output package.

  • Remove Manager terminology in favor of types that represent what they output. A package consumer could create an output type by saying output.JSON

  • Remove the Flush() and Put() methods on each Outputter in favor of a single Output(). Everywhere in the codebase we were just ranging over the CheckResults collection to Put() them into the output manager and then flushing. This lets us just pass in a collection of results and write them e.g.

outputter := output.Get(format)
if err := outputter.Output(results); err != nil {
  return fmt.Errorf("output results: %w", err)
}
  • Replace log.Logger with io.Writer. This gives us, as well as package consumers, way more flexibility in how they can get at the formatted output. Tests can continue to pass inbytes.Buffer, and the Conftest binary can pass in os.Stdout.

Signed-off-by: John Reese <john@reese.dev>
Copy link
Collaborator

@Blokje5 Blokje5 left a comment

Choose a reason for hiding this comment

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

LGTM! The Output interface is a lot cleaner. It is nice to remove the need for the clients of the package to be aware of the order of calling methods.

@Blokje5 Blokje5 merged commit d31f0c8 into open-policy-agent:master Sep 22, 2020
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.

2 participants