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

Input: comment and severity support #827

Merged
merged 7 commits into from Jul 24, 2020
Merged

Conversation

Niraj-Kamdar
Copy link
Contributor

@Niraj-Kamdar Niraj-Kamdar commented Jul 18, 2020

Fixes: #486

@codecov-commenter
Copy link

codecov-commenter commented Jul 18, 2020

Codecov Report

Merging #827 into master will decrease coverage by 5.29%.
The diff coverage is 93.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #827      +/-   ##
==========================================
- Coverage   88.73%   83.44%   -5.30%     
==========================================
  Files         156      156              
  Lines        2566     2567       +1     
  Branches      279      278       -1     
==========================================
- Hits         2277     2142     -135     
- Misses        224      355     +131     
- Partials       65       70       +5     
Flag Coverage Δ
#longtests 83.44% <93.02%> (-5.30%) ⬇️

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

Impacted Files Coverage Δ
cve_bin_tool/cli.py 79.13% <71.42%> (-5.09%) ⬇️
cve_bin_tool/cve_scanner.py 80.86% <92.30%> (+0.13%) ⬆️
cve_bin_tool/input_engine.py 98.33% <100.00%> (+4.89%) ⬆️
cve_bin_tool/output_engine/__init__.py 94.82% <100.00%> (-0.09%) ⬇️
cve_bin_tool/output_engine/console.py 100.00% <100.00%> (ø)
cve_bin_tool/output_engine/html.py 17.64% <100.00%> (ø)
cve_bin_tool/output_engine/util.py 100.00% <100.00%> (ø)
test/test_input_engine.py 100.00% <100.00%> (ø)
test/test_output_engine.py 97.64% <100.00%> (-0.09%) ⬇️
cve_bin_tool/cvedb.py 53.84% <0.00%> (-32.06%) ⬇️
... and 9 more

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 e57d7fd...7e2dba2. Read the comment docs.

@Niraj-Kamdar Niraj-Kamdar force-pushed the input1 branch 4 times, most recently from a3e506f to e6170b4 Compare July 18, 2020 06:54
fix bug
no unnecessary change
not nec
fix space
fix bugs

def __init__(
self, filename: str, logger: Logger = None, error_mode=ErrorMode.TruncTrace
):
self.filename = os.path.abspath(filename)
self.logger = logger or LOGGER.getChild(self.__class__.__name__)
self.error_mode = error_mode
self.parsed_data = {}
self.parsed_data = defaultdict(dict)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change data structure to incorporate more triage data like comments and custom severity

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put a comment about this a few useful places, such as line 35 above? The type hints are good but they don't quite tell the full story about what we're expecting to have in there.

@@ -33,6 +29,7 @@ def output_csv(all_cve_data, outfile):
"cve_number",
"severity",
"remarks",
"comments",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

comments support :)

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.

Looks good, just some nits about comments really.


def __init__(
self, filename: str, logger: Logger = None, error_mode=ErrorMode.TruncTrace
):
self.filename = os.path.abspath(filename)
self.logger = logger or LOGGER.getChild(self.__class__.__name__)
self.error_mode = error_mode
self.parsed_data = {}
self.parsed_data = defaultdict(dict)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put a comment about this a few useful places, such as line 35 above? The type hints are good but they don't quite tell the full story about what we're expecting to have in there.


def parse_input(self) -> Dict[tuple, Remarks]:
def parse_input(self) -> DefaultDict[tuple, Dict[str, Dict[str, Any]]]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Another place where remarks told us a lot more than Dict[str, Dict[str, any]] does. Should we be using something like triage_data to replace remarks for clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, We can construct our own type. TriageData and that will solve problem.

Comment on lines +18 to +19
# TriageData is dictionary of cve_number mapped to dictionary of remarks, comments and custom severity
TriageData = Dict[str, Dict[str, Any]]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created alias for TriageData datastructure.

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 think this addresses my concern. Just waiting on CI before merging.

@terriko terriko merged commit 6c3388f into intel:master Jul 24, 2020
@Niraj-Kamdar Niraj-Kamdar deleted the input1 branch July 24, 2020 07:00
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.

Allowing for triage of cve-bin-tool results
3 participants