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

feat(checkers): add pre-commit hook for reformatting checkers table #1290

Merged
merged 8 commits into from
Aug 3, 2021

Conversation

imsahil007
Copy link
Contributor

No description provided.

@codecov-commenter
Copy link

Codecov Report

Merging #1290 (f97c4a3) into main (2d69f84) will decrease coverage by 0.79%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1290      +/-   ##
==========================================
- Coverage   78.96%   78.16%   -0.80%     
==========================================
  Files         262      263       +1     
  Lines        4801     4846      +45     
  Branches      578      589      +11     
==========================================
- Hits         3791     3788       -3     
- Misses        856      904      +48     
  Partials      154      154              
Flag Coverage Δ
longtests 78.16% <0.00%> (-0.80%) ⬇️

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

Impacted Files Coverage Δ
cve_bin_tool/format_checkers.py 0.00% <0.00%> (ø)
cve_bin_tool/async_utils.py 91.66% <0.00%> (-2.09%) ⬇️

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 2d69f84...f97c4a3. Read the comment docs.

Copy link
Contributor

@Molkree Molkree left a comment

Choose a reason for hiding this comment

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

I like that it'll be automated now! 👍

  1. Maybe add a remark about this hook in the checkers readme after the line "Once the checker is added, its name should also be added to __init__.py", something like "If you have pre-commit installed your next commit will trigger pre-commit [hook](link to this hook) to automatically update checkers tables, or you can run it manually with pre-commit run format_checkers

  2. Maybe add total number of checkers to the table? Should be easy (it's over 100 now!)

  3. I wanted to see how the table with not full last row will look but right now there are exactly 7 * 15 checkers heh. So I went to double check and I see two files for curl, curl.py and libcurl.py. Both point to the same CVE references but I'm not sure they are different tools or not (one is lib, another is CLI tool?). Only curl is in the checker list though, libcurl is not there. @terriko, am I missing something? Either this checker should be added to the list or one file removed?

  4. When I try to run this hook I get an error, any idea what might be wrong?
    image

(cve-bin-tool-venv) molkree@DESKTOP-V2SIETN:~/cve-bin-tool$ python -m cve_bin_tool -V
/home/molkree/cve-bin-tool-venv/bin/python: No module named cve_bin_tool.__main__; 'cve_bin_tool' is a package and cannot be directly executed
(cve-bin-tool-venv) molkree@DESKTOP-V2SIETN:~/cve-bin-tool$ python -m cve_bin_tool.cli -V
2.2

cve_bin_tool/format_checkers.py Outdated Show resolved Hide resolved
cve_bin_tool/format_checkers.py Outdated Show resolved Hide resolved
cve_bin_tool/format_checkers.py Outdated Show resolved Hide resolved

def max_checker_length(checkers):
"""Returns a list of max length of each column"""
checkers[-1].extend([""] * (CHECKERS_TABLE_SIZE - len(checkers[-1])))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Molkree This should handle the case you mentioned

@imsahil007
Copy link
Contributor Author

I like that it'll be automated now!

  1. Maybe add a remark about this hook in the checkers readme after the line "Once the checker is added, its name should also be added to __init__.py", something like "If you have pre-commit installed your next commit will trigger pre-commit [hook](link to this hook) to automatically update checkers tables, or you can run it manually with pre-commit run format_checkers
  2. Maybe add total number of checkers to the table? Should be easy (it's over 100 now!)
  3. I wanted to see how the table with not full last row will look but right now there are exactly 7 * 15 checkers heh. So I went to double check and I see two files for curl, curl.py and libcurl.py. Both point to the same CVE references but I'm not sure they are different tools or not (one is lib, another is CLI tool?). Only curl is in the checker list though, libcurl is not there. @terriko, am I missing something? Either this checker should be added to the list or one file removed?
  4. When I try to run this hook I get an error, any idea what might be wrong?
    image
(cve-bin-tool-venv) molkree@DESKTOP-V2SIETN:~/cve-bin-tool$ python -m cve_bin_tool -V
/home/molkree/cve-bin-tool-venv/bin/python: No module named cve_bin_tool.__main__; 'cve_bin_tool' is a package and cannot be directly executed
(cve-bin-tool-venv) molkree@DESKTOP-V2SIETN:~/cve-bin-tool$ python -m cve_bin_tool.cli -V
2.2

This is weird. Have you tried running pip install -e .?

@Molkree
Copy link
Contributor

Molkree commented Jul 29, 2021

Have you tried running pip install -e .?

Yes.

@terriko
Copy link
Contributor

terriko commented Aug 3, 2021

Broke this with the other pre-commit merge. I've tried to fix via web interface (which is often a disaster for black because it's hard to see whitespace correctly) so you might want to verify the fix.

@terriko
Copy link
Contributor

terriko commented Aug 3, 2021

Answer for @Molkree :

Yes, libcurl.py and curl.py used to refer to command line tool vs library. I believe they got merged to just curl.py when @Niraj-Kamdar refactored the checkers last year so it should detect both, but I'll open an issue to look at it and likely remove the file.

@terriko terriko mentioned this pull request Aug 3, 2021
Copy link
Contributor

@terriko terriko left a comment

Choose a reason for hiding this comment

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

I'm not sure why it's not working for @Molkree but it's working for me and seems to be working in CI. We may get some weirdness when I merge it because I merged some checkers before merging this.

@terriko terriko merged commit 0333caa into intel:main Aug 3, 2021
@Molkree
Copy link
Contributor

Molkree commented Aug 5, 2021

seems to be working in CI

Actually, it's not added to CI now, and it probably should be.

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.

4 participants