-
Notifications
You must be signed in to change notification settings - Fork 443
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
feat: merged report content change and comments added in html reports #2913
Conversation
Sounds like a nice improvement! I've approved the tests to run and I'll be back to do more of a code review after they've finished. But some first comments: it would be nice to have an explicit test of the behaviour and a note about it in MANUAL.md or the how tos on merged reports so people know what to expect. |
Codecov Report
@@ Coverage Diff @@
## main #2913 +/- ##
==========================================
- Coverage 82.39% 82.37% -0.02%
==========================================
Files 676 676
Lines 10660 10662 +2
Branches 1429 1429
==========================================
Hits 8783 8783
Misses 1501 1501
- Partials 376 378 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Merged report logic explained
Hi @terriko, thank you for the feedback. I updated the MANUAL.md to explain the logic behind merged reports. Im not sure what to do for the test you mentioned, can you explain ? |
I've approved the tests to run again. The docs changes look clear enough. I was thinking a a test that showed that a comment was plumbed through and displayed properly in the final output, so something like this:
Basically just a basic regression test showing that comments get displayed is all I had in mind. Mostly so if someone accidentally makes it so they're not displayed in the future, the test suite will fail. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two things:
-
Looks like the spell checker doesn't recognize the word "cybersecurity" -- you can either change it to just "security" (I think that's sufficiently clear in context) or add the word to https://github.com/intel/cve-bin-tool/blob/main/.github/actions/spelling/allow.txt
-
Black (our code linter/pep8 style enforcer) is complaining. We've got instructions for all the linters here: https://github.com/intel/cve-bin-tool/blob/main/CONTRIBUTING.md#running-linters but for black only you probably just need to
pip install black
andblack $filename
on the couple of python files you changed. It'll auto-fix whatever it's complaining about, at which point you can add the updated version to the pull request. It's usually whitespace or line length nitpicky stuff.
Let me know if this all seems like too much work -- I can absolutely do these for you, but most people prefer to run them themselves so I don't mess with their branches.
change of term cybersecurity -> security
Hi @terriko, I added the test you asked for. Let me know if its all good 😄 |
I've approved the tests to run, but assuming they pass I think this is ready to merge! |
@terriko seems like "Windows tests (pull_request)" was cancelled. How can I help? |
Windows tests timing out are probably not your fault. They take 3-4x as long as the linux ones, and even with a long timeout there are occasionally days when they exceed the running time due to issues outside our control. Usually if they time out, I go through to check and make sure that most of the tests ran and see where it got stuck to see if it's something we can resolve, but half the time it's just "azure cloud was slow that day." In this case, the HTML tests are run in a separate job so that's the one to watch and make sure that it passes. I've approved the tests to run again, but it looks like we're having some other problems today (e.g. #2969 and some cache failures) so they may fail and need me to restart them after the cache has updated properly. |
Hi @terriko seems like all test passed, do I need to do anything? Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about the delay! Let's get this merged!
Merged reports will contain all CVEs encountered at their last version (Remarks, comments...).
Now comments show up in the html reports (print and interavtive mode)
This is my first pull request, if i made mistakes let me know and I'm sorry :)
Thanks you