contrib: Add dynamic HTML viewer

[martinpitt]

This is similar in spirit to rpminspect-report, but much more
lightweight. It does not require any build steps, and can just be copied
as-is to any web server.

---

Case: Real-world bodhi result

This bodhi result looks like this:

http://localhost:8000/viewer.html?url=https://artifacts.dev.testing-farm.io/1b160c72-a94c-4ea0-a187-0afb0a24950a/work-rpminspectD7I35E/rpminspect/tree/results.json#

---

The permissions inspection has more than one result type, and you can filter:

---

Expanding diagnostics:

Case: Failing and small result (synthetic)

To get some "interesting" results, I ram rpmdiff on a known-bad dummy RPM:

rpminspect -F json -c /usr/share/rpminspect/fedora.yaml -r "Fedora 37" ~/upstream/cockpit-ostree/test/files/chrony-0.1-2.noarch.rpm > contrib/chrony.json

then edited the rpmdeps result to VERIFY (to cover that as well). Then http://localhost:8000/viewer.html?url=./chrony.json looks like this:

[martinpitt]

@dcantrell , @bookwar: This is a 2-hour PoC as per our recent discussion -- i.e. styling, functionality etc. is all wide open. But at this point I'm not entirely sure where we want to go with this. It feels to me that an rpmdiff job in Fedora/RHEL/etc. should spit out that json file and additionally copy viewer.html (possibly as index.html) next to it, so that it appears as an artifact that is browsable by humans. Then it can co-evolve with the data format in rpminspect, and be tested/developed independently of TF/dashboard.

[msrb]

I think this is a great start. Thanks!

But at this point I'm not entirely sure where we want to go with this. It feels to me that an rpmdiff job in Fedora/RHEL/etc. should spit out that json file and additionally copy viewer.html (possibly as index.html) next to it,

I think this is exactly what we want to do. I.e. we don't want rpminspect itself to create the HTML report.

Big +1 from me 😉

[dcantrell]

This is really nice, thank you. The actual tool generating the end results was a problem I had with rpmdiff. The core program in rpmdiff actually spat out XML that could only be interpreted by the rpmdiff web frontend. I wanted to split the inspection tool from the results viewing as much as possible, which is why I did the different output formats in rpminspect. But that was never meant to have rpminspect outputting HTML directly.

This is a nice addition and I could see putting it in the rpminspect package and we could instruct layered products on how to use it to view results in their own pipelines.

[jimbair]

Only nitpick feedback I have is I see RPMInspect in the screenshots at the top/header, and I know that won't fly ;) It should be all lowercase: rpminspect

[martinpitt]

@jimbair: Oh, sure! I took that from rpminspect-report, but trivial to change of course. Done.

[martinpitt]

Talked to @msrb. Notes for myself:

• Run this on a pair of old/new rpm with some meaningful diff
• If old version already has a broken license, and the new one did not change that, does the report show that or not? IOW, is this a "pure delta", or does comparison merely enable some additional checks (such as file size changes)
• Think about how to represent the delta aspect in a useful manner; look at old rpmdiff reports
• Possibly: there might be OK/INFO results with additional details; do we need to make them stand out/more discoverable/filterable?

[dcantrell]

Talked to @msrb. Notes for myself:

* Run this on a pair of old/new rpm with some meaningful diff

* If old version already has a broken license, and the new one did not change that, does the report show that or not? IOW, is this a "pure delta", or does comparison merely enable some _additional_ checks (such as file size changes)

Inspections are divided in to two categories: those that apply to a single build and those that apply to both a single build or comparison. File size change reporting only makes sense when comparing builds, for instance. In the case of the license inspection, it only looks at the after build because it is an inspection that is for a single build. It does not report license changes from the before package to the after package, only whether or not the after package is correct.

You can see the categorization in lib/inspect.c in the inspections array.

* Think about how to represent the delta aspect in a useful manner; look at old rpmdiff reports

But be careful with that. Old rpmdiff reporting was not the best. The biggest request from users now is to be able to see the grid view that rpmdiff presented. If each inspection was in a grid and then color code the cell based on whether or not it passes or doesn't. With the result codes in rpminspect results, the color coding could be more fine-grained. Like OK could be green, VERIFY could be yellow, BAD could be red. INFO could be lunar. Just as an example.

* Possibly: there might be OK/INFO results with additional details; do we need to make them stand out/more discoverable/filterable?

rpminspect carries three types of information in addition to the result and waiver flag (the waiver flag is either "not waivable", "waivable by anyone", or "waiver requires security team response"...these are lifted from rpmdiff and are entirely up to the front end to implement in the web display. The reporting on the rpminspect side is just providing information for further manipulation by a web viewer.

The three pieces of information in a result are the message (what did rpminspect find), details (this sometimes includes a command that was run, unified diff output, and so on...but it could also be empty), and a remedy (the remedy was not in rpmdiff and was something I wanted rpminspect to do similar to setroubleshoot for SELinux).

[msrb]

This is how I (roughly) envision the result page could look like:

[martinpitt]

Thanks @dcantrell for the details! This helps indeed. @msrb , I'll add such a table to the viewer and keep the current list, so that we have several views to play around with. Note that I'm working on TF today, but I'll look at this this week.

[martinpitt]

@msrb , @dcantrell : I implemented such a "summary" table with clickable inspections, which are then shown below. That summary requires some non-trivial computation, as the JSON does not have an overall result per inspection (but not too bad). The table adjusts to the available screen width (please try it).

The presentation of the details is exactly the same as in the previous list view. Of course, if we want to go with that approach, it can become less dense, and more vertically oriented. For now, this is mostly a design study which overall approach we want to use. Do you see a need to support both? (with a switch like "show details of all inspections")

Screenshots in description are up to date.

[martinpitt]

Note to self: I created a fake-rpms.py which produces pairs of RPMs for various scenarios (empty, bad license, growing file). With that, I get comparison checks such as

  "filesize": [
    {   
      "result": "INFO",
      "waiver authorization": "Not Waivable",
      "message": "\/file.txt grew by 909000% on noarch"
    },  
    {   
      "result": "OK",
      "waiver authorization": "Not Waivable"
    }   
  ]

[martinpitt]

I dropped the long "list" view, and only kept the lego brick table and the details view for a particular test. I think this is overall more useful, and it matches what @msrb asked for above. The details view is still mostly unstyled, I'd really appreciate some suggestions how it should look like? This is mostly visual design, and I'm not great at it 🙈

I added "remedy" as well.

I also ran it against a comparison of a synthetic rpm pair (see the script above)

rpminspect -F json -o contrib/filechange.json -c /usr/share/rpminspect/fedora.yaml -r 'Fedora 37' /var/tmp/repo/filechange-1.0* 

http://localhost:8000/viewer.html?url=./filechange.json#

or the "rpmdeps" test:

@dcantrell , @msrb , WDYT?

[msrb]

@martinpitt thanks!

I had a chance to show the viewer to a couple of people yesterday and the overall feedback was very positive :)

Few suggestions:

• the table of inspections could be static, i.e. filtering would only affect the test output, but the table would still show overall result for everything; wdyt?
• colors: OK could be green and INFO could be blue-ish. That would be more aligned to what rpmdiff output looks like today.
  • DIAG is new, and kinda special. If INFO is blue, then DIAG should probably use some other color, not sure which one to be honest :)
• it would be great to also show "SKIPPED" inspections in the table (gray color?), but I know that they are simply not listed in the result.json right now; maybe this would be an RFE for rpminspect?
  • having skipped inspections visible in the output could make the life easier for QE as it would be immediately obvious that some of the inspections were turned off somewhere in the configuration

cc also @dcantrell

What do you guys think?

[martinpitt]

@msrb : Fine for me to not filter the top table, that's up to you and the users; even the full set with OK is manageable (see the very last screenshot). Then the filter checkboxes should move out of the header and below the table, as they would then only affect the individual test results. And I propose to only show it if there is more than one result type (i.e. they are not all INFO or all OK and such).

I'm happy to take the same colors as the rpmdiff results. I suppose the mapping would be like this: passed → OK, Info → INFO, needs inspection → VERIFY, failed → BAD. I'll see how DIAG fits in.

"SKIPPED" is something that would need to happen in the JSON output itself, the viewer should not make assumptions about which tests exist.

[martinpitt]

@msrb, @dcantrell : Reworked according to the above. At this point I am not sure if the filtering even still makes sense, it's a bit corner-case-ish. The only really long one is "rpmdeps" , which has tons of "INFO" and a single "OK", so filtering wouldn't give you much. And for such small cases it seems overkill:

It was fun to implement, but the filtering needs quite a lot of logic.

[martinpitt]

@dcantrell : While we sort out the remaining UI issues, do you have some structural advice/preferences? In the more recent iterations I split out .js and .css from the viewer.html file -- mostly because vim tends to get confused when you do large jumps through the code, and it can't tell any more whether it's in a HTML, CSS, or JS section. But if you prefer, I'm happy to merge this back into a single file. Any reference other than README? (I suppose I'll need to add this to some Makefile.am so that it gets disted)

[msrb]

@martinpitt I'd vote for having it merged back into a single file. That would make it (slightly) easier to copy around in CI :) But even if it stays the way it is now, it's not a big deal.

[martinpitt]

Ack -- I merged the files back together, and squashed the fixups. I have no other TODOs from my side, please keep them coming. 😁 Ready for review, anyway.

BTW, the original zuul report from the description is gone. I used a current bodhi one now:

http://localhost:8000/viewer.html?url=https://artifacts.dev.testing-farm.io/1b160c72-a94c-4ea0-a187-0afb0a24950a/work-rpminspectD7I35E/rpminspect/tree/results.json#

This log path is not discoverable at all. I made a TODO item (for the TF, probably gluetools-modules?) to add this results.json as a proper log link, so that it will appear no the TF results page. This will also be the place to link to viewer.html in the future.

[msrb]

@dcantrell How do you like the current version? LGTM? :)

It's a +1 from me.

[dcantrell]

How can I look at this locally? None of these links work (obviously the localhost one won't). Or, better, can someone else set this up at an actual URL that I can go to and see what this thing is doing?

I can't comment until I can see what the current iteration looks like. I have comments around the colors I see in these screenshots as well as some of the text, but I'd like to look at the whole thing to get a better idea.

[msrb]

@dcantrell Here you go:
https://msrb.github.io/viewer.html?url=https://msrb.github.io/results.json

[dcantrell]

OK, I figured out how to use it locally. Some comments:

• The colors are a problem for me. I am color blind and I can't distinguish between a lot of these. OK and VERIFY look the same and INFO has no color. All in all, the colors are very faint. For me, sharp bright colors are best and even then it's a gamble if I can even see it. I would prefer the color you see on the GitHub Comment button or the "Rebase and merge" button for OK results. For VERIFY results I would prefer dark yellow. BAD results should be dark red (or a darker red) with bright white text because black text on dark red is unreadable. A light blue for INFO results is good. If you're using that now, I can't see it. It looks white. I like the color you have for 'diagnostics' and feel that would be better for INFO results. More info: https://www.datylon.com/blog/data-visualization-for-colorblind-readers
• I recommend pulling the diagnostics results out of the grid. It's not an inspection. I would like to see that as a clickable link as part of the heading. That makes it clear that it's not part of the results even though that's how the information comes out of rpminspect. Also, the diagnostics output does not need to show "Not Waivable" but I have to add that as part of the output for the results. I could modify that on the rpminspect side.
• Make the text in the cells in the table left-aligned.
• Grid lines for the table would be nice.
• For BAD and VERIFY results, making the inspection name bold would help it stand out.

Today I am working on the SKIP support to show skipped/disabled inspections as skipped rather than just not having them reported. Having those reported in the grid as a grey cell with black text would be nice. I should have that work done today.

Other than the above comments, I really like this viewer. It's very similar to what users saw with rpmdiff but it's modern and compatible with rpminspect.

rpminspect itself has inspection descriptions if you list inspections (-l) in verbose mode (-v). The rpmdiff web viewer showed test descriptions and I'm not sure that would be helpful here. It's probably sufficient to just link to the rpminspect documentation.

[dcantrell]

Oh, also strike the "contrib: " from the commit summary lines. I have a set of prefixes I prefer to use for git commit messages in doc/git.md. This is used to generate release information about changes. For this PR, I recommend using the "[cmd]" prefix because this is adding an HTML viewer for the rpminspect command's results output.

[martinpitt]

Thanks @dcantrell I did have a stronger color schema before, you can still see it here in an older screenshot. I was asked to take the colors from rpmdiff as they appeared too strong. Thanks for the datylon link! I'll read that. Adding non-color hints like bold and grid lines also sounds good.

I'll work on your suggestions next week, I'm on PTO for the rest of this week.

[martinpitt]

.. Err, I wanted to ask -- were the older colors better for you? But anyway, you already gave color recommendations, so I'll use these.

[dcantrell]

@martinpitt Yes, the older colors you had were better for me. Finding the right balance is tricky because colorblindness is not a single thing. Ask any 5 colorblind people to label colors and you'll get 5 different responses. For me, very faint or light colors are the worst. There's not enough there for me to distinguish it. I find that having alternative distinguishing features like bold or italics or all capital letters gives a secondary way to read the information while still preserving color indicators for other people.

Hope PTO was good. I am at the All Things Open conference next week, but will probably occasionally check email.

[martinpitt]

I installed the Let's get color blind Firefox extension, and indeed the colors are bad for all three types of color blindness. I did your items about bold text, grid lines, colors, and did some other minor cleanups. At least to me, contrast appears much better now:

Full color:

Red blindness:

Green blindness:

Blue blindness:

I'll add back the diagnostics as a separate thing next (and update the description screenshots), this is mostly just to capture the new color schema.

[martinpitt]

I pulled out the diagnostics from the table into an expander at the top. Screenshots in description are fully up to date. PTAL?

[dcantrell]

This looks much better now. Really cleaned up and sharp. Thank you for the updates. I've looked through everything and have no other comments. I'm ready to merge.

[martinpitt]

Nice, thanks @dcantrell for the review! I had expected some more lose ends here, like adding this to dist tarballs in meson.build or so -- but I just see this doesn't happen for the other bits in contrib/ either. We can just copy it out of git in our infrastructure, like TF does for its generic results viewer as well.