Add results info to the status annotation and status cmd output #829
Conversation
|
Still in a WIP phase but the gist of the logic is here. Let me know what you think. |
Codecov Report
@@ Coverage Diff @@
## master #829 +/- ##
==========================================
- Coverage 44.68% 44.44% -0.25%
==========================================
Files 75 76 +1
Lines 4565 4687 +122
==========================================
+ Hits 2040 2083 +43
- Misses 2392 2473 +81
+ Partials 133 131 -2
Continue to review full report at Codecov.
|
| } | ||
|
|
||
| for i := range podStatus.Plugins { | ||
| if podStatus.Plugins[i].Plugin == pluginType { |
There was a problem hiding this comment.
Is the plugin result type unique per run? I think I remember seeing that it was but it's slightly confusing to me here because podStatus.Plugins[i].Plugin seems like it would be the plugin's name. I know that our built in plugins use the same values for the name and the result type though.
There was a problem hiding this comment.
Yes the type is unique; agree that name/type duality is confusing and I want to fix that. See #830 ; someone reported an issue which was caused because of that too.
|
I think this is looking good so far. I agree with your comment about adding a |
Going to review the code myself for cleanup, but this information is available as part of the status on the object so I think this is already pretty easy with this library (there are methods to get the status). Just not to get this info from a tarball without the pod. That can be a TBD I think. |
|
|
||
| fmt.Fprintf(tw, "PLUGIN\tNODE\tSTATUS\n") |
There was a problem hiding this comment.
The lack of a trailing \t caused this last column to not be aligned at all
| func printAll(w io.Writer, status *aggregation.Status) error { | ||
| tw := tabwriter.NewWriter(w, 1, 8, 1, '\t', tabwriter.AlignRight) |
There was a problem hiding this comment.
The alignment was ignored since the padding char was \t, it also seemed to make it impossible to slightly tweak the output in a clear fashion. Changed this out for space padding.
|
|
||
| // Effectively making a pivot chart to count the unique combinations of status/result. | ||
| statusResultKey := func(p aggregation.PluginStatus) string { | ||
| return p.Status + ":" + p.ResultStatus |
There was a problem hiding this comment.
We were just iterating over values and rolling on on some value before (status). Now we roll up on the tuple (status, resultStatus).
| @@ -161,23 +206,6 @@ func printSummary(w io.Writer, status *aggregation.Status) error { | |||
| return nil | |||
| } | |||
|
|
|||
| type pluginSummaries []pluginSummary | |||
There was a problem hiding this comment.
FYI: moved this type data to the top of the page; somewhat of a nit/preference but I have always understood types to be defined at the top of the file under constants/variables.
| @@ -94,7 +95,7 @@ func TestPrintStatus(t *testing.T) { | |||
| } | |||
|
|
|||
| if b.String() != test.expected { | |||
| t.Errorf("expected output to be %q, got %q", test.expected, b.String()) | |||
There was a problem hiding this comment.
Tweaked this so that each of "expected" and "got" would start on a new line; just thought it looked more clear at a glance.
johnSchnake
changed the title
Recently we added some post-processing which would allow users to analyze arbitrary plugins for how many tests passed/failed/etc. This change adds some of that information into the status annotation which we put onto the aggregator pod. This will allow people to check results before they choose to download the tarball. In addition, we added a bit mnore metadata into that object describing the tarball size/name/sha which may be useful for users wanting to confirm the data that received or save the metadata alongside the tarball. Fixes #819 Signed-off-by: John Schnake <[email protected]>
What this PR does / why we need it:
Which issue(s) this PR fixes
Special notes for your reviewer:
Example output as of now:
TODO:
statuscommand to show some of this infototalfield to the result counts? It would make it so people dont have to add them, but it also is a special case that wouldn't really be a count of a particular status so I'm not sure it is a great idea.Release note: