Remove deprecated code #433

emayuri-godaddy · 2023-01-13T00:27:49Z

To help us get this pull request reviewed and merged quickly, please be sure to include the following items:

Tests (if applicable)
Documentation (if applicable)
Changelog entry
A full explanation here in the PR description of the work done

PR Type

What kind of change does this PR introduce?

Backward Compatibility

Is this change backward compatible with the most recently released version? Does it introduce changes which might change the user experience in any way? Does it alter the API in any way?

Yes (backward compatible)
No (breaking changes)

Issue Linking

What's new?

Removed deprecated flags rules, b64 and hex, and corresponding code around deprecated options.

rbailey-godaddy

Nits.

Mostly, beef up the changelog entry for people who have forgotten what things we dropped... The rest is complaints about the way you specified tuples in various test methods. Just specify the tuples like before, but with dict members instead of str members:

my_tuple = ({"my": "dict"},)

rbailey-godaddy · 2023-01-13T00:33:07Z

CHANGELOG.md

@@ -2,6 +2,8 @@ vX.X.X - DD MMM YYYY
 --------------------

 Features:
+* [#433](https://github.com/godaddy/tartufo/pull/433) - Dropped support for deprecated flags from v3.0


In this context, and in the interest of visibility, I would explicitly list the removed flags here again... for people who only read the CHANGELOG and not the error messages. ;-)

rbailey-godaddy · 2023-01-13T00:41:12Z

tests/test_base_scanner.py

+        self.options.include_path_patterns = tuple(
+            [{"path-pattern": "foo", "reason": "Testing exclude path pattern"}]
+        )


This works but I had to read it 3 times and then try it myself to convince myself it did what was needed. How about:

Suggested change

self.options.include_path_patterns = tuple(

[{"path-pattern": "foo", "reason": "Testing exclude path pattern"}]

)

self.options.include_path_patterns = (

{"path-pattern": "foo", "reason": "Testing exclude path pattern"},

)

rbailey-godaddy · 2023-01-13T00:42:28Z

tests/test_base_scanner.py

+        self.options.exclude_path_patterns = tuple(
+            [{"path-pattern": "foo", "reason": "Testing exclude path pattern"}]
+        )


Ditto above. The only reason I comment is that it isn't obvious to the casual reader that you're getting a tuple consisting of a single dict vs a tuple consisting of a single list

rbailey-godaddy · 2023-01-13T00:42:46Z

tests/test_base_scanner.py

+        self.options.exclude_signatures = tuple(
+            [{"signature": "foo", "reason": "Testing exclude signature"}]
+        )


...and again

rbailey-godaddy · 2023-01-13T00:42:57Z

tests/test_base_scanner.py

+        self.options.exclude_signatures = tuple(
+            [{"signature": "foo", "reason": "Testing exclude signature"}]
+        )


rbailey-godaddy · 2023-01-13T00:44:47Z

tests/test_git_repo_scanner.py

+        self.global_options.include_path_patterns = tuple(
+            [{"path-pattern": "foo/", "reason": "Inclusion reason"}]


Suggested change

self.global_options.include_path_patterns = tuple(

[{"path-pattern": "foo/", "reason": "Inclusion reason"}]

self.global_options.include_path_patterns = (

{"path-pattern": "foo/", "reason": "Inclusion reason"},

tests/test_git_repo_scanner.py

rbailey-godaddy · 2023-01-13T00:45:37Z

tests/test_git_repo_scanner.py

+        self.global_options.exclude_path_patterns = tuple(
+            [{"path-pattern": "bar/", "reason": "Exclusion reason"}]


Suggested change

self.global_options.exclude_path_patterns = tuple(

[{"path-pattern": "bar/", "reason": "Exclusion reason"}]

self.global_options.exclude_path_patterns = (

{"path-pattern": "bar/", "reason": "Exclusion reason"},

tests/test_git_repo_scanner.py

rbailey-godaddy · 2023-01-13T00:46:23Z

tests/test_git_repo_scanner.py

+        self.global_options.exclude_signatures = tuple(
+            [{"signature": "bar", "reason": "Reason to exclude signature"}]


Suggested change

self.global_options.exclude_signatures = tuple(

[{"signature": "bar", "reason": "Reason to exclude signature"}]

self.global_options.exclude_signatures = (

{"signature": "bar", "reason": "Reason to exclude signature"},

sushantmimani

You missed these:

tartufo/tartufo/config.py

Lines 190 to 194 in 7f3f026

    
           warnings.warn( 
        
               "Storing rules in a separate file is deprecated and will be removed " 
        
               "in tartufo 4.x. Please use the 'rule-patterns' config " 
        
               " option instead.", 
        
               DeprecationWarning,

tartufo/tartufo/scanner.py

Lines 606 to 610 in 7f3f026

    
           warnings.warn( 
        
               f"Signature {old_signature} was generated by an old version of tartufo and is deprecated. " 
        
               "tartufo 4.x will not recognize this signature. " 
        
               f"Please update your configuration to use signature {new_signature} instead.", 
        
               DeprecationWarning,

emayuri-godaddy · 2023-01-13T17:21:04Z

You missed these:

tartufo/tartufo/config.py

Lines 190 to 194 in 7f3f026

warnings.warn(

"Storing rules in a separate file is deprecated and will be removed "

"in tartufo 4.x. Please use the 'rule-patterns' config "

" option instead.",

DeprecationWarning,

tartufo/tartufo/scanner.py

Lines 606 to 610 in 7f3f026

warnings.warn(

f"Signature {old_signature} was generated by an old version of tartufo and is deprecated. "

"tartufo 4.x will not recognize this signature. "

f"Please update your configuration to use signature {new_signature} instead.",

DeprecationWarning,

Working on addressing second comment here

…git diff

dclayton-godaddy · 2023-01-13T21:14:29Z

tests/test_git_repo_scanner.py

        test_scanner = scanner.GitRepoScanner(
            self.global_options, self.git_options, "."
        )
        self.assertEqual(test_scanner.excluded_signatures, ("bar/",))

-    def test_error_is_not_raised_when_two_styles_signatures_are_configured(self):
+    def test_error_is_raised_when_two_styles_signatures_are_configured(self):


Suggested change

def test_error_is_raised_when_two_styles_signatures_are_configured(self):

def test_error_is_raised_when_string_signature_is_used(self):

dclayton-godaddy · 2023-01-13T21:15:37Z

tests/test_git_repo_scanner.py

        test_scanner = scanner.GitRepoScanner(
            self.global_options, self.git_options, "."
        )
        self.assertEqual(test_scanner.included_paths, [re.compile("bar/")])

-    def test_error_is_not_raised_when_two_styles_included_paths_are_configured(self):
+    def test_error_is_raised_when_two_styles_included_paths_are_configured(self):


Suggested change

def test_error_is_raised_when_two_styles_included_paths_are_configured(self):

def test_error_is_raised_when_string_included_path_is_used(self):

dclayton-godaddy · 2023-01-13T21:16:18Z

tests/test_git_repo_scanner.py

        test_scanner = scanner.GitRepoScanner(
            self.global_options, self.git_options, "."
        )
        self.assertEqual(test_scanner.excluded_paths, [re.compile("bar/")])

    @mock.patch("tartufo.scanner.GitScanner.filter_submodules", mock.MagicMock())
-    def test_error_is_not_raised_when_two_styles_excluded_paths_are_configured(self):
+    def test_error_is_raised_when_two_styles_excluded_paths_are_configured(self):


Suggested change

def test_error_is_raised_when_two_styles_excluded_paths_are_configured(self):

def test_error_is_raised_when_string_exclude_path_is_used(self):

dclayton-godaddy · 2023-01-13T21:17:44Z

tests/test_git_repo_scanner.py

        self.global_options.exclude_signatures = [
            "foo/",
            {"signature": "bar/", "reason": "path pattern"},
        ]
        test_scanner = scanner.GitRepoScanner(
            self.global_options, self.git_options, "."
        )
-        self.assertCountEqual(test_scanner.excluded_signatures, ("foo/", "bar/"))
+        with self.assertRaisesRegex(
+            ConfigException, "signature is illegal in exclude-signatures"


Suggested change

ConfigException, "signature is illegal in exclude-signatures"

ConfigException, "str signature is illegal in exclude-signatures"

sushantmimani · 2023-01-13T21:18:51Z

tartufo/scanner.py


    def evaluate_entropy_string(
        self,
        chunk: types.Chunk,
        line: str,
        string: str,
        min_entropy_score: float,
-        backwards_compatibility_prefix: Optional[str],


Docstring and documentation should be updated

dclayton-godaddy

Left a few suggestions. Otherwise looks good.

…_prefix

Remove deprecated code

c5d846f

emayuri-godaddy requested a review from a team as a code owner January 13, 2023 00:27

emayuri-godaddy requested review from sushantmimani and rbailey-godaddy January 13, 2023 00:27

Updating changelog

7f3f026

rbailey-godaddy requested changes Jan 13, 2023

View reviewed changes

sushantmimani requested changes Jan 13, 2023

View reviewed changes

Addressing review comments

a2ae532

Removed support for old signatures which generated with +/- chars in …

b6015f7

…git diff

emayuri-godaddy requested review from sushantmimani and rbailey-godaddy January 13, 2023 20:57

dclayton-godaddy reviewed Jan 13, 2023

View reviewed changes

sushantmimani reviewed Jan 13, 2023

View reviewed changes

dclayton-godaddy reviewed Jan 13, 2023

View reviewed changes

Fixing test names and removed comments around backwards_compatibility…

411c12b

…_prefix

emayuri-godaddy requested review from sushantmimani and dclayton-godaddy January 13, 2023 21:53

rbailey-godaddy approved these changes Jan 13, 2023

View reviewed changes

sushantmimani approved these changes Jan 13, 2023

View reviewed changes

emayuri-godaddy merged commit 24976f1 into main Jan 13, 2023

emayuri-godaddy deleted the tartufo_v4_prep branch January 13, 2023 23:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove deprecated code #433

Remove deprecated code #433

emayuri-godaddy commented Jan 13, 2023 •

edited

Loading

rbailey-godaddy left a comment

rbailey-godaddy Jan 13, 2023

sushantmimani Jan 13, 2023

rbailey-godaddy Jan 13, 2023

rbailey-godaddy Jan 13, 2023

rbailey-godaddy Jan 13, 2023

rbailey-godaddy Jan 13, 2023

rbailey-godaddy Jan 13, 2023

rbailey-godaddy Jan 13, 2023

rbailey-godaddy Jan 13, 2023

sushantmimani left a comment •

edited

Loading

emayuri-godaddy commented Jan 13, 2023

dclayton-godaddy Jan 13, 2023

dclayton-godaddy Jan 13, 2023 •

edited

Loading

dclayton-godaddy Jan 13, 2023

dclayton-godaddy Jan 13, 2023

sushantmimani Jan 13, 2023 •

edited

Loading

dclayton-godaddy left a comment

		self.global_options.include_path_patterns = tuple(
		[{"path-pattern": "foo/", "reason": "Inclusion reason"}]

		self.global_options.exclude_path_patterns = tuple(
		[{"path-pattern": "bar/", "reason": "Exclusion reason"}]

		self.global_options.exclude_signatures = tuple(
		[{"signature": "bar", "reason": "Reason to exclude signature"}]

	warnings.warn(
	"Storing rules in a separate file is deprecated and will be removed "
	"in tartufo 4.x. Please use the 'rule-patterns' config "
	" option instead.",
	DeprecationWarning,

	warnings.warn(
	f"Signature {old_signature} was generated by an old version of tartufo and is deprecated. "
	"tartufo 4.x will not recognize this signature. "
	f"Please update your configuration to use signature {new_signature} instead.",
	DeprecationWarning,

	def test_error_is_raised_when_two_styles_signatures_are_configured(self):
	def test_error_is_raised_when_string_signature_is_used(self):

	def test_error_is_raised_when_two_styles_included_paths_are_configured(self):
	def test_error_is_raised_when_string_included_path_is_used(self):

	def test_error_is_raised_when_two_styles_excluded_paths_are_configured(self):
	def test_error_is_raised_when_string_exclude_path_is_used(self):

	ConfigException, "signature is illegal in exclude-signatures"
	ConfigException, "str signature is illegal in exclude-signatures"

Remove deprecated code #433

Remove deprecated code #433

Conversation

emayuri-godaddy commented Jan 13, 2023 • edited Loading

PR Type

Backward Compatibility

Issue Linking

What's new?

rbailey-godaddy 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

Choose a reason for hiding this comment

sushantmimani left a comment • edited Loading

Choose a reason for hiding this comment

emayuri-godaddy commented Jan 13, 2023

Choose a reason for hiding this comment

dclayton-godaddy Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sushantmimani Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

dclayton-godaddy left a comment

Choose a reason for hiding this comment

emayuri-godaddy commented Jan 13, 2023 •

edited

Loading

sushantmimani left a comment •

edited

Loading

dclayton-godaddy Jan 13, 2023 •

edited

Loading

sushantmimani Jan 13, 2023 •

edited

Loading