doc: improve new contributor documentation

[terriko]

Ready for review now!

• fixes: Update contributor docs to recommend tests & docs with each new feature #1465
• fixes: Update contributor docs to talk about conventional commits #1317
• fixes: Moving contributor guide to expected location #1446

[codecov-commenter]

Codecov Report

Merging #1467 (8cab9e6) into main (d2c1e27) will decrease coverage by 0.09%.
The diff coverage is 44.44%.

@@            Coverage Diff             @@
##             main    #1467      +/-   ##
==========================================
- Coverage   82.44%   82.34%   -0.10%     
==========================================
  Files         279      279              
  Lines        5458     5473      +15     
  Branches      886      888       +2     
==========================================
+ Hits         4500     4507       +7     
- Misses        767      773       +6     
- Partials      191      193       +2     

Flag	Coverage Δ
longtests	82.34% <44.44%> (-0.10%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cve_bin_tool/cli.py	71.75% <43.75%> (-1.64%)	⬇️
cve_bin_tool/cvedb.py	82.22% <50.00%> (-0.19%)	⬇️

---

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 161aeec...8cab9e6. Read the comment docs.

[terriko]

I'm honestly not sure why spell check is failing right now. Here's the error message:

Preparing a comment for pull_request
https://api.github.com/repos/intel/cve-bin-tool/issues/1467/comments
curl: (6) Could not resolve host: null

I think it doesn't like that I renamed the contributor document? But it could bea network error? Re-running immediately did nothing so I'll give it another go tomorrow. if it doesn't work I can always revert the rename and put it in a separate PR.

[terriko]

Hm, taking the file rename commit out does not appear to have resolved the problem.

[terriko]

@anthonyharrison and @pdxjohnny I think I'm done writing for now. If you've got a bit of time, I'd appreciate a review from either or both you to make sure I didn't miss anything we want new contributors to know.

[anthonyharrison]

Then you can type cve-binary-tool on the command line and it will do the right thing. This includes some special code intended to deal with adding new checkers to the list on the fly so things should work seamlessly for you while you're building new contributions.

cve-binary-tool should be cve-bin-tool

• isort sorts imports alphabetically and by type
• black provides automatic style formatting. This will give you basic PEP8 compliance. (PEP8 is where the default python style guide is defined.)
• flake8 provides additional code "linting" for more complex errors like unused imports.

Should also mention pyupgrade

We usually merge pull requests into a single commit when we accept them, so it's fine if you have lots of commits in your branch while you figure stuff out, and we can fix your commit message as needed then. But if you make sure that at least the title of your pull request follows the Conventional Commits format that you'd like for that merged commit message, that makes our job easier!

In addition to conventional commits formatting, should we also mention that the Issue number should also be included in the commit message e.g. feat: Add great new feature (Fixes #1234)

GENERAL

I think we need to have something more in the Limitations section of the README which explicitly recognises some of the limitations of the tool. Running the tool does not guarantee that it will detect all of the vulnerabilities (it is dependent on the checkers which are available and if there isn't a checker for a specific library, vulnerabilities in that library will not be detected) nor does it guarantee that any of the reported vulnerabilities are present/exploitable. As with all tools there will be some false reporting, both positive and negative although this can be controllied via the triage report which allows reported CVEs to be suppressed. Probably also worth emphasising the importance of keeping the vulnerability database up to date (there is a statement to that effect in the offline HOW TO) but might be worth repeating here as well.

[terriko]

I believe I've addressed @anthonyharrison 's comments above on the cve-binary-tool typo, pyupgrade, and the github keywords for linking issues. I took a stab at the limitations section, but I feel like it could probably use another pass so I'm going to go ahead and file a separate ticket to look at that again:

• related: Improve "Limitations" Section in readme #1496

In the interest of having the new contributor doc easy for new contributors to actually read, I'm going to merge this before I head out on a short new year's vacation. But further feedback is always welcome!

And yes, it looks like I may have triggered the spelling issue again; I'm going to ignore it for now -- the spelling issues were fixed but it's still trying to comment even though it shouldn't be?