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

Prints all plugins when invoking sonobuoy results by default #909

Merged
merged 1 commit into from
Sep 27, 2019
Merged

Prints all plugins when invoking sonobuoy results by default #909

merged 1 commit into from
Sep 27, 2019

Conversation

johnSchnake
Copy link
Contributor

What this PR does / why we need it:
If given a tarball, there is not a way to detect which plugins
were actually run without opening it up first. This means that the
sonobuoy results command is less useful since you have to have
preexisting knowledge about the tarball/run. You'd also have to
manually execute the command numerous times if you want all the
summaries of the plugins.

This change:

  • makes the default value for the --plugin flag the
    empty string
  • when a string value is given, it will print a single report
    like the existing behavior
  • when the default, empty string is used, we will list the report
    for each plugin
  • adds a new file, meta/info.json which will contain the list of
    plugins that were actually run so that it is easier to grab this
    list (rather than more complicated logic using the other data in
    the tarball)

Which issue(s) this PR fixes
Fixes #905

Special notes for your reviewer:
If you target an old tarball with the new CLI you'll get a message about the plugins not being discernable from either the flags or the meta/info.json. I could fallback to trying the e2e by default at that point but I think it is a bit confusing to twist the default behavior since the error message in some cases may not reflect what was the intended behavior (e.g. they want to list all plugins and then get an error msg about e2e)

If you target a new tarball with an old CLI, no difference in behavior.

If you target a new tarball (e.g. a tarball made by the new image) with the new CLI then you'll get the new behavior.

Release note:

`sonobuoy results results.tar.gz` now defaults to printing information on all the plugins that were run. You may still print just the e2e plugin information by setting the existing flag `--plugin e2e`.

@johnSchnake johnSchnake requested a review from zubron September 26, 2019 16:47
@@ -66,7 +67,11 @@ func NewCmdResults() *cobra.Command {
},
Args: cobra.ExactArgs(1),
}
AddPluginFlag(&data.plugin, cmd.Flags())
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, FYI, this was inappropriate to use here. The values --plugin -p were ok, but the description of the flags meaning was specific to the sonobuoy images command.

pkg/config/config.go Outdated Show resolved Hide resolved
If given a tarball, there is not a way to detect which plugins
were actually run without opening it up first. This means that the
`sonobuoy results` command is less useful since you have to have
preexisting knowledge about the tarball/run. You'd also have to
manually execute the command numerous times if you want all the
summaries of the plugins.

This change:
 -  makes the default value for the `--plugin` flag the
empty string
 - when a string value is given, it will print a single report
like the existing behavior
 - when the default, empty string is used, we will list the report
for each plugin
 - adds a new file, meta/info.json which will contain the list of
plugins that were actually run so that it is easier to grab this
list (rather than more complicated logic using the other data in
the tarball)

Fixes #905

Signed-off-by: John Schnake <[email protected]>
@codecov-io
Copy link

Codecov Report

Merging #909 into master will decrease coverage by 0.11%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #909      +/-   ##
==========================================
- Coverage   46.86%   46.74%   -0.12%     
==========================================
  Files          76       76              
  Lines        5100     5138      +38     
==========================================
+ Hits         2390     2402      +12     
- Misses       2574     2597      +23     
- Partials      136      139       +3
Impacted Files Coverage Δ
pkg/discovery/discovery.go 7.61% <0%> (-0.33%) ⬇️
pkg/client/results/reader.go 55.9% <0%> (-0.71%) ⬇️
cmd/sonobuoy/app/results.go 70.98% <44.82%> (-5.88%) ⬇️

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 aae48ef...29cef7c. Read the comment docs.


// InfoFile contains data not that isn't strictly in another location
// but still relevent to post-processing or understanding the run in some way.
InfoFile = "info.json"
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that a method is provided to return the info file path, does it need to be exported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you raise a good question which I definitley touched on when coding this: who is in charge of what information and what is the right way to grab it?

A few different packages hardcode these string values (results.json, errors.json, results/, etc), some of the values here are exported, some arent. Some have methods to return their values, some dont.

I'm going to make a new ticket specifically for fixing this up and clarifying where to find all the results structure (folders/names/etc) and how to use them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#912

TIL: there is a quick 'Reference comment in new issue' button which will create a new issue with the comment body.

@johnSchnake johnSchnake merged commit d9ca4ba into vmware-tanzu:master Sep 27, 2019
@johnSchnake johnSchnake deleted the resultsAllPlugins branch September 27, 2019 15:25
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.

Sonobuoy results command should list all plugins
3 participants