New style entropy exclusions #229

tarkatronic · 2021-10-14T15:40:05Z

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?

Reverted the behavior of --exclude-entropy-patterns to behave as it did before allow exclude-entropy-patterns to match lines containing partial matches #222 by default
Moved the new behavior introduced by --exclude-entropy-patterns behind an updated config specification style
Updated our method of showing DeprecationWarnings

dclayton-godaddy · 2021-10-14T15:51:08Z

tartufo/scanner.py

+                match = match and rule.path_pattern.match(path) is not None
+        elif rule.re_match_type == "search":
+            if rule.pattern:
+                match = rule.pattern.search(line) is not None


It seems like we're mixing two terms. Match type should be search vs match. However match_type seems to be changing the match scope from string to line. Should we provide match_scope (entropy vs line)?

Yeah... I agree the naming is crap here. But this was my third iteration, and this split logic will only exist until v3.0, so I gave up on better naming. I'd be open to suggestions if you think it really is that bad.

If someone uses search, this change will search by line. Will this introduce a breaking change for 3.0? As in, will 3.0 no longer use search to search by line? If so and it's easy I'd recommend we add something like match-scope to avoid a breaking change.

+1 to this. It seems like it would be easy enough to use caller-supplied input to drive .re_match_type, and default to the behavior you have now. The benefit of doing it now rather than waiting for 3.0 is that users who actually pay attention to the deprecation warning could rewrite their configs in a way that would preserve behavior while making it explicit what they intended.

In fact, at the risk of making you tear out your hair, it might be better to allow (making this up on the fly) scope: {line|string} and transfer this directly to .re_match_type (or .re_scope if you prefer), where "match" becomes "string" and "search" becomes "line". Then we are aligned more with the intent vs the actual implementation, and it's ever so slightly easier to remember why we're doing this. ;-)

I think this can be handled in a follow-up easily enough, if it's desired badly enough. As-is, with the existing deprecation warning, users will be able to update their configs for the upcoming behavioral change.

dclayton-godaddy · 2021-10-14T15:53:16Z

docs/source/features.rst

+.. code-block:: toml
+
+    [tool.tartufo]
+    exclude-entropy-patterns = [


dclayton-godaddy · 2021-10-14T15:53:59Z

docs/source/features.rst

+
+    [tool.tartufo]
+    exclude-entropy-patterns = [
+        {files = 'docs/.*\.md$', match = '^[a-zA-Z0-9]$', reason = 'exclude all git SHAs in the docs'},


Should we be using the term pattern where it's used in other places?

Good callout! I can make that change. That also reminds me that I forgot to add to the documentation, that's the only field that's absolutely required!

dclayton-godaddy · 2021-10-14T17:24:57Z

tartufo/scanner.py

+                match = match and rule.path_pattern.match(path) is not None
+        elif rule.re_match_type == "search":
+            if rule.pattern:
+                match = rule.pattern.search(line) is not None


If someone uses search, this change will search by line. Will this introduce a breaking change for 3.0? As in, will 3.0 no longer use search to search by line? If so and it's easy I'd recommend we add something like match-scope to avoid a breaking change.

dclayton-godaddy · 2021-10-14T17:27:19Z

docs/source/features.rst

+.. code-block:: toml
+
+    [[tool.tartufo.exclude-entropy-patterns]]
+    files = 'docs/.*\.md$'


should this be file-pattern?

or path-pattern?

Hmmmm yeah I think maybe file-pattern would be good, since we're using it for matching file names.

Although now that I look at the code, it's already referred to as path_pattern, so that seems to be the natural winner. 😄

rbailey-godaddy

I made a few gratuitous suggestions, but the current code isn't bad. I'd lobby for the tweaks if the time is available, but if we're trying to get a quick fix out the door, go for it.

rbailey-godaddy · 2021-10-15T00:31:46Z

tartufo/config.py

                name=rule_name,
                pattern=re.compile(rule_definition["pattern"]),
                path_pattern=re.compile(path_pattern) if path_pattern else None,
+                re_match_type="match",
            )
        except AttributeError:
            rule = Rule(
-                name=rule_name, pattern=re.compile(rule_definition), path_pattern=None
+                name=rule_name,
+                pattern=re.compile(rule_definition),
+                path_pattern=None,
+                re_match_type="match",


I think your life would become much simpler if you used re.compile("") instead of None for the case that path_pattern is not specified. The empty regex matches immediately, which is what you're looking for, and the exact result of the match test is never used.

Given that this part is largely pre-existing code/behavior, I will make note of this for when we go back over this in preparation for 3.0

rbailey-godaddy · 2021-10-15T00:34:02Z

tartufo/config.py

@@ -255,20 +260,47 @@ def compile_rule(pattern: str) -> Rule:
        path, pattern = pattern.split("::", 1)
    except ValueError:  # Raised when the split separator is not found
        path = ".*"


Same here. I would use "" instead of ".*", as it (also) matches every target every time, but doesn't require scanning the target string.

rbailey-godaddy · 2021-10-15T00:34:33Z

tartufo/config.py

+                Rule(
+                    name=pattern.get("reason", None),  # type: ignore[union-attr]
+                    pattern=re.compile(pattern["pattern"]),  # type: ignore[index]
+                    path_pattern=re.compile(pattern.get("files", ".*")),  # type: ignore[union-attr]


Suggested change

path_pattern=re.compile(pattern.get("files", ".*")), # type: ignore[union-attr]

path_pattern=re.compile(pattern.get("files", "")), # type: ignore[union-attr]

For the same reasons as above

rbailey-godaddy · 2021-10-15T00:37:30Z

tartufo/scanner.py

+            if rule.pattern:
+                match = rule.pattern.match(string) is not None
+            if rule.path_pattern:
+                match = match and rule.path_pattern.match(path) is not None


Because rule.pattern is mandatory and rule.path_pattern is always a re (if you took the advice above), I think this can be simplified:

Suggested change

if rule.pattern:

match = rule.pattern.match(string) is not None

if rule.path_pattern:

match = match and rule.path_pattern.match(path) is not None

match = rule.pattern.match(string) is not None and rule.path_pattern.match(path) is not None

or presumably even match = bool(rule.pattern.match(string) and rule.path_pattern.match(path))

rbailey-godaddy · 2021-10-15T00:40:56Z

tartufo/scanner.py

+                match = match and rule.path_pattern.match(path) is not None
+        elif rule.re_match_type == "search":
+            if rule.pattern:
+                match = rule.pattern.search(line) is not None


+1 to this. It seems like it would be easy enough to use caller-supplied input to drive .re_match_type, and default to the behavior you have now. The benefit of doing it now rather than waiting for 3.0 is that users who actually pay attention to the deprecation warning could rewrite their configs in a way that would preserve behavior while making it explicit what they intended.

rbailey-godaddy · 2021-10-15T00:45:50Z

tartufo/scanner.py

+                match = match and rule.path_pattern.match(path) is not None
+        elif rule.re_match_type == "search":
+            if rule.pattern:
+                match = rule.pattern.search(line) is not None


In fact, at the risk of making you tear out your hair, it might be better to allow (making this up on the fly) scope: {line|string} and transfer this directly to .re_match_type (or .re_scope if you prefer), where "match" becomes "string" and "search" becomes "line". Then we are aligned more with the intent vs the actual implementation, and it's ever so slightly easier to remember why we're doing this. ;-)

smimani-godaddy

LGTM 🦅

mayuriesha

LGTM

rbailey-godaddy

Still LGTM ;)

tarkatronic requested a review from a team as a code owner October 14, 2021 15:40

tarkatronic requested review from djlarsu, jmink-godaddy and rbailey-godaddy October 14, 2021 15:40

dclayton-godaddy reviewed Oct 14, 2021

View reviewed changes

tarkatronic added 6 commits October 14, 2021 11:04

Implement new-style exclusion matching & deprecate old

5e696ef

Fix up the spelling list

062f9f1

Fix up test coverage

d33d712

Make deprecation messages more uniform & explicit.

f3bed55

Add changelog entries for the new changes

843041c

Document the new way for exclude-entropy-patterns

8764ba7

tarkatronic force-pushed the new-style-entropy-exclusions branch from 6a3c2ee to 8764ba7 Compare October 14, 2021 17:05

Rename "match" -> "pattern"

31e01fe

dclayton-godaddy reviewed Oct 14, 2021

View reviewed changes

rbailey-godaddy approved these changes Oct 15, 2021

View reviewed changes

Rename "files" -> "path-pattern" for entropy exclusions

da39c80

tarkatronic requested review from emayuri-godaddy and sushantmimani October 18, 2021 21:51

Merge branch 'main' into new-style-entropy-exclusions

3d9d534

smimani-godaddy approved these changes Oct 18, 2021

View reviewed changes

sushantmimani approved these changes Oct 18, 2021

View reviewed changes

mayuriesha approved these changes Oct 18, 2021

View reviewed changes

rbailey-godaddy approved these changes Oct 19, 2021

View reviewed changes

tarkatronic merged commit 2731713 into main Oct 19, 2021

tarkatronic deleted the new-style-entropy-exclusions branch October 19, 2021 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New style entropy exclusions #229

New style entropy exclusions #229

tarkatronic commented Oct 14, 2021 •

edited

Loading

dclayton-godaddy Oct 14, 2021

tarkatronic Oct 14, 2021

dclayton-godaddy Oct 14, 2021

rbailey-godaddy Oct 15, 2021

rbailey-godaddy Oct 15, 2021

tarkatronic Oct 18, 2021

dclayton-godaddy Oct 14, 2021

dclayton-godaddy Oct 14, 2021

tarkatronic Oct 14, 2021

dclayton-godaddy Oct 14, 2021

dclayton-godaddy Oct 14, 2021

dclayton-godaddy Oct 14, 2021

tarkatronic Oct 18, 2021

tarkatronic Oct 18, 2021

rbailey-godaddy left a comment

rbailey-godaddy Oct 15, 2021

tarkatronic Oct 18, 2021

rbailey-godaddy Oct 15, 2021

rbailey-godaddy Oct 15, 2021

rbailey-godaddy Oct 15, 2021

rbailey-godaddy Oct 15, 2021

rbailey-godaddy Oct 15, 2021

smimani-godaddy left a comment

mayuriesha left a comment

rbailey-godaddy left a comment

	path_pattern=re.compile(pattern.get("files", ".*")), # type: ignore[union-attr]
	path_pattern=re.compile(pattern.get("files", "")), # type: ignore[union-attr]

New style entropy exclusions #229

New style entropy exclusions #229

Conversation

tarkatronic commented Oct 14, 2021 • edited Loading

PR Type

Backward Compatibility

Issue Linking

What's new?

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

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

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

smimani-godaddy left a comment

Choose a reason for hiding this comment

mayuriesha left a comment

Choose a reason for hiding this comment

rbailey-godaddy left a comment

Choose a reason for hiding this comment

tarkatronic commented Oct 14, 2021 •

edited

Loading