add -e --exclude arument for excluding path #876

imsahil007 · 2020-08-18T06:56:13Z

Closes #819
In reference to: #874
@Niraj-Kamdar

codecov-commenter · 2020-08-18T07:00:12Z

Codecov Report

Merging #876 into master will increase coverage by 2.25%.
The diff coverage is 80.95%.

@@            Coverage Diff             @@
##           master     #876      +/-   ##
==========================================
+ Coverage   84.70%   86.96%   +2.25%     
==========================================
  Files         167      167              
  Lines        2765     2784      +19     
  Branches      298      302       +4     
==========================================
+ Hits         2342     2421      +79     
+ Misses        344      292      -52     
+ Partials       79       71       -8

Flag	Coverage Δ
#longtests	`86.96% <80.95%> (+2.25%)`	⬆️

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

Impacted Files	Coverage Δ
test/test_config.py	`0.00% <ø> (ø)`
test/test_cli.py	`96.00% <76.47%> (+15.54%)`	⬆️
cve_bin_tool/cli.py	`86.46% <100.00%> (+3.13%)`	⬆️
cve_bin_tool/version_scanner.py	`86.20% <100.00%> (+1.85%)`	⬆️
test/test_extractor.py	`97.80% <0.00%> (ø)`
cve_bin_tool/cvedb.py	`85.03% <0.00%> (+0.78%)`	⬆️
cve_bin_tool/extractor.py	`60.62% <0.00%> (+3.14%)`	⬆️
test/test_scanner.py	`91.01% <0.00%> (+11.23%)`	⬆️
... and 3 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 51696f0...b028714. Read the comment docs.

terriko

This looks like it's heading in the right direction, but I'd like to have some tests for it before we merge anything.

imsahil007 · 2020-08-19T05:56:37Z

This looks like it's heading in the right direction, but I'd like to have some tests for it before we merge anything.

What kind of tests does this argument require?

Niraj-Kamdar · 2020-08-19T11:53:48Z

This looks like it's heading in the right direction, but I'd like to have some tests for it before we merge anything.

What kind of tests does this argument require?

You need to add tests in test_cli that make sure that excluded files or directories aren't being scanned but first You need to make it fnmatch compatible because users generally use Unix style pattern to exclude directory and files.

Niraj-Kamdar · 2020-08-19T11:55:55Z

cve_bin_tool/util.py

@@ -150,6 +150,9 @@ def walk(self, roots=None):
                    for i in filenames
                    if self.pattern_match(i, self.pattern)
                    and not self.pattern_match(i, self.file_exclude_pattern)
+                    and not self.directory_pattern(


Why are we still using directory_pattern? Is fix mentioned by me in previous PR not working?

Why are we still using directory_pattern? Is fix mentioned by me in the previous PR not working?

It wasn't working because there was not an * after the path which is to be excluded. It may contain any no. of the path. So, I thought it will be better just to match the string beginning. It won't work otherwise. fnmatch takes a regex type of input. If we were to exclude /home/temp/ in a directory /home/ .
We will have to append /home/temp/ with an *. So, that it will work for all the sub-dir as well as files inside that dir

Yes we can easily append that by checking

if not string.endswith("*"): string += "*"

If you use fnmatch user can specify path like /home/temp/*.jpg this will only ignore jpg instead of whole directory. There are also other regex patterns supported by fnmatch.

@Niraj-Kamdar Check now

Niraj-Kamdar

Overall it looks good just couple of small changes.

Niraj-Kamdar · 2020-08-21T16:48:22Z

cve_bin_tool/util.py

                    if self.pattern_match(i, self.pattern)
                    and not self.pattern_match(i, self.file_exclude_pattern)


Let's also replace i with os.path.join(dirpath, i) here and dirnames also because It was a bug in actual code.
It would be good if you rename this i variable to something useful like filename and dirname.

Niraj-Kamdar · 2020-08-21T16:49:22Z

cve_bin_tool/cli.py

+            if args["exclude"]:
+                version_scanner = VersionScanner(
+                    should_extract=args["extract"],
+                    exclude_folders=args["exclude"],
+                    error_mode=error_mode,
+                )
+            else:
+                version_scanner = VersionScanner(
+                    should_extract=args["extract"], error_mode=error_mode
+                )


You don't need to add if...else... here, since by default it will be an empty list which is valid.

Niraj-Kamdar · 2020-08-21T16:57:00Z

cve_bin_tool/version_scanner.py

+        self.exclude_folders = exclude_folders
+        self.walker = DirWalk(
+            folder_exclude_pattern="".join(
+                [exclude + "*;" for exclude in exclude_folders]
+            )
+        ).walk


self.exclude_folders = exclude_folders self.exclude_folder.append(".git") self.walker = DirWalk( folder_exclude_pattern=";".join(exclude if exclude.endswith("*") else exclude + "*" in exclude_folders) ).walk

Here we should include ".git" by default and we have to check if "*" doesn't exist in the end because if it exist there will be two stars at the end.

Niraj-Kamdar · 2020-08-21T16:57:42Z

test/test_cli.py

+        """ Test that the exclude paths are not scanned """
+        test_path = os.path.abspath(os.path.dirname(__file__))
+        exclude_path = os.path.join(test_path, "assets/")
+        checkers = ["curl", "libcurl", "kerberos", "kerberos_5"]


You have to check that scanner isn't detecting any checker when we exclude a path.

You can get list of all checkers from VersionScanner class

Did all the changes

Have you forgotten to push?

Niraj-Kamdar · 2020-08-24T10:19:57Z

cve_bin_tool/util.py

+                    if self.pattern_match(dirname, self.folder_include_pattern)
+                    and not self.pattern_match(dirname, self.folder_exclude_pattern)


You need to join path here also os.path.join(dirpath, dirname).

Niraj-Kamdar

Awesome looks nice. Thanks for this again.

imsahil007 · 2020-08-24T11:45:19Z

Awesome looks nice. Thanks for this again.

Thank you! You have been very helpful and guiding.

terriko · 2020-09-02T16:30:32Z

Thank you! Sorry it took a little bit to get the final merge in.

imsahil007 · 2020-09-02T16:34:41Z

Thank you! Sorry it took a little bit to get the final merge in.

Your welcome. It's okay, not an issue!

terriko requested changes Aug 19, 2020

View reviewed changes

terriko added the awaiting changes label Aug 19, 2020

Niraj-Kamdar reviewed Aug 19, 2020

View reviewed changes

imsahil007 force-pushed the exclude branch 3 times, most recently from 8475f2b to e4f6a44 Compare August 20, 2020 19:23

Niraj-Kamdar suggested changes Aug 21, 2020

View reviewed changes

imsahil007 added 5 commits August 21, 2020 23:24

add exclude arg

1c4aabf

exclude for compressed files in sub directory

e9ccde4

exclude_path works with fnmatch

3bd6370

add tests for --exclude

116ce95

exclude path [.git] appended

3df7f87

imsahil007 force-pushed the exclude branch from e4f6a44 to 3df7f87 Compare August 21, 2020 18:27

imsahil007 requested a review from terriko August 23, 2020 13:55

changes in --exclude

ca31ba3

Niraj-Kamdar suggested changes Aug 24, 2020

View reviewed changes

join path in dirnames

b028714

Niraj-Kamdar approved these changes Aug 24, 2020

View reviewed changes

terriko merged commit dec65fb into intel:master Sep 2, 2020

imsahil007 deleted the exclude branch October 5, 2020 18:30

imsahil007 mentioned this pull request Oct 25, 2020

Add no-extract option, update documentation about -x being deprecated #913

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add -e --exclude arument for excluding path #876

add -e --exclude arument for excluding path #876

imsahil007 commented Aug 18, 2020

codecov-commenter commented Aug 18, 2020 •

edited

Loading

terriko left a comment

imsahil007 commented Aug 19, 2020

Niraj-Kamdar commented Aug 19, 2020

Niraj-Kamdar Aug 19, 2020

imsahil007 Aug 19, 2020 •

edited

Loading

Niraj-Kamdar Aug 19, 2020

imsahil007 Aug 21, 2020

Niraj-Kamdar left a comment

Niraj-Kamdar Aug 21, 2020

Niraj-Kamdar Aug 21, 2020

Niraj-Kamdar Aug 21, 2020

Niraj-Kamdar Aug 21, 2020

Niraj-Kamdar Aug 21, 2020

imsahil007 Aug 23, 2020

Niraj-Kamdar Aug 24, 2020

Niraj-Kamdar Aug 24, 2020

Niraj-Kamdar left a comment

imsahil007 commented Aug 24, 2020

terriko commented Sep 2, 2020

imsahil007 commented Sep 2, 2020

		if self.pattern_match(i, self.pattern)
		and not self.pattern_match(i, self.file_exclude_pattern)

		if self.pattern_match(dirname, self.folder_include_pattern)
		and not self.pattern_match(dirname, self.folder_exclude_pattern)

add -e --exclude arument for excluding path #876

add -e --exclude arument for excluding path #876

Conversation

imsahil007 commented Aug 18, 2020

codecov-commenter commented Aug 18, 2020 • edited Loading

Codecov Report

terriko left a comment

Choose a reason for hiding this comment

imsahil007 commented Aug 19, 2020

Niraj-Kamdar commented Aug 19, 2020

Choose a reason for hiding this comment

imsahil007 Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Niraj-Kamdar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Niraj-Kamdar left a comment

Choose a reason for hiding this comment

imsahil007 commented Aug 24, 2020

terriko commented Sep 2, 2020

imsahil007 commented Sep 2, 2020

codecov-commenter commented Aug 18, 2020 •

edited

Loading

imsahil007 Aug 19, 2020 •

edited

Loading