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

issue #50: exposes OutputResults functionality #51

Closed
wants to merge 3 commits into from

Conversation

robertojrojas
Copy link
Contributor

Clients can re-use the util.OutputResults function.
Here is an example:

outcfg := &util.OutputConfig{
  OutputFile:        outputFile,
  NoRemediations:    noRemediations,
  IncludeTestOutput: includeTestOutput,
}
err = util.OutputResults(controls, summary, outcfg)
...

@codecov-io
Copy link

codecov-io commented Aug 13, 2019

Codecov Report

Merging #51 into master will increase coverage by 4.55%.
The diff coverage is 63.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
+ Coverage   58.38%   62.93%   +4.55%     
==========================================
  Files           5        5              
  Lines         507      518      +11     
==========================================
+ Hits          296      326      +30     
+ Misses        186      156      -30     
- Partials       25       36      +11
Impacted Files Coverage Δ
util/util.go 41.66% <63.63%> (+21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de62def...581aa0f. Read the comment docs.

Copy link
Contributor

@lizrice lizrice left a comment

Choose a reason for hiding this comment

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

This is great as far as it goes, but I have a couple of suggestions for even better :-)

// OutputConfig represents values to be passed to
// the OutputResults function.
type OutputConfig struct {
JSONFmt bool
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be fine to start with, but I think it would be better to have an Outputter interface because there might be more options in future. In fact kube-bench already has regular output, JSON output, and an option to write output to a postgres database. Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm... that's a great idea. I can definitely see the need to send the output to NoSQL dbs and the interface would make it easier to add different implementations. One thing I'm of is how to strike a balance between bench-common providing default Outputters while allowing *-bench specific extensions.

Fail: 0,
Warn: 2,
}
err := OutputResults(controls, summary, outcfg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Bit of an optional extra but it would be nice to test that the json output is as expected

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking twice about this, the test is highlighting the problem of a filename being passed in instead of io.Writer

@lizrice
Copy link
Contributor

lizrice commented Oct 11, 2019

This is now superceded by #52

@lizrice lizrice closed this Oct 11, 2019
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.

None yet

3 participants