feat(controller): support filtering Git subscriptions using globs and path prefixes #1778
Conversation
✅ Deploy Preview for docs-kargo-akuity-io ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1778 +/- ##
==========================================
+ Coverage 44.89% 46.14% +1.25%
==========================================
Files 213 213
Lines 13624 13868 +244
==========================================
+ Hits 6116 6399 +283
+ Misses 7244 7183 -61
- Partials 264 286 +22 ☔ View full report in Codecov by Sentry. |
|
@maksimstankevic thanks for this! This looks pretty good. I've got just a few miscellaneous observations and questions.
|
|
|
Ok. So second point is the one most in need of addressing. imho, yes... prefix and glob match should go together, but I am willing to hear arguments against that. My argument in favor of it is that anyone accustomed to working in a shell environment is likely to find that to be the least surprising behavior. I contend most people would never think to do |
|
One argument against combining them would be that there could be potential conflicts or overlaps in certain scenarios, such as when dealing with literals versus special characters, nested patterns, or character escaping. To solve this, one would have to start defining (and documenting) rules around precedence. Which in the end may make it easier from both a technical and user perspective to create a separation between the two. |
|
Yeah, I agree, in my eyes that would be an overcomplication for a simple topic. I would go with "regex:/regexp:" prefix for regexps, "glob:" prefix for globs, these first 2 being the advanced options for folks wishing to use them (like me 😄 ) and strings without prefix would be for exact matches and string prefixes like "foo/" to match "foo/bar". |
|
I'm a little confused because I feel as if we all probably use prefixes and globs on a regular basis in our shells without ever having to specify which we're using. We also use both in |
|
There are small details to take into account here:
|
Right... but that's an implementation detail. You don't think about that when you write commands like |
|
But that implementation detail means that globbing > prefix matches. Which means there is an order of precedence. |
That may technically be so, but how often do you stop and think about this? That is to say, don't we all possess an abstract intuition about how patterns are matched that doesn't require us to think so deeply about precedence? Would you not find it surprising if |
Not very often, but the shell also present me with different things to e.g. do literal string interpretations. Assuming I have a directory called These kind of details make me worry about edge cases for directory structures and (not so) great people do while naming things. In addition to it creating technical hurdles over simply using an identifier, as you can't e.g. say "if it contains one of these special characters, it's a glob".
It would depend on what the API tells me the features are, if the default is just simple prefix matching and there also is a regular expression feature with an identifier. I wouldn't, I think. |
This is a fair point, but I also want to default to the least surprising behavior for the benefit of those who didn't read the manual. That is the main thing I care about. If we think prefix matching would be the least surprising default, let's go with that and use the |
|
@maksimstankevic as always, thank you for your patience and your efforts! |
…e Git Subscription Signed-off-by: Maksim Stankevic <maksim.stankevic1@gmail.com>
Signed-off-by: Maksim Stankevic <maksim.stankevic1@gmail.com>
maksimstankevic
force-pushed
the
feat/1711
branch
from
b185f03 to
2d3947c
Compare
| @@ -303,19 +306,19 @@ func ignores(tagName string, ignore []string) bool { | |||
| return false | |||
| } | |||
|
|
|||
| // pathsFilterPositive returns true when IncludePaths and/or ExcludePaths | |||
| // matchesPathsFilters returns true when IncludePaths and/or ExcludePaths | |||
| // filters match one or more commit diffs and new Freight is | |||
| // to be produced. It returns false otherwise. | |||
| func matchesPathsFilters(includePaths []string, excludePaths []string, diffs []string) (bool, error) { | |||
There was a problem hiding this comment.
This function looks like it could be written more simply and more efficiently as well.
Something like the following pseudocode would probably be easier to understand and perform better:
for diff in diffs:
if !match(diff, includePrefixes) && !match(diff, includeGlobs) && !match(diff, includeRegexes):
// Diff not selected by any of the patterns
continue
// If we get to here, the diff was selected by one of the patterns. Check if it's un-selected.
if match(diff, excludePrefixes) || match(dif, excludeGlobs) || match(diff, excludeRegexes):
// Diff was unselected by one of the patterns
continue
// If we get to here, we have found one path that was selected by the include patterns
// and not un-selected by the exclude patterns. That's enough to say we found a change
// that should be allowed to produce Freight.
return true
The benefits of this implementation are that it returns from the entire function as soon as it finds even one path they is selected and not un-selected. It also leverages short-circuiting in the boolean logic so that we don't ever check against more patterns than we need to.
There was a problem hiding this comment.
@krancour absolutely, change pushed, I need to improve on my code-owner spirit :)
There was a problem hiding this comment.
Think in addition to this, it may be good to make this interface based. I outlined an approach like this in hiddeco@43573b7.
The advantage of this is that include and exclude rules can maintain their declared order of precedence.
There was a problem hiding this comment.
@maksimstankevic it looks like these changes don't avoid as many unnecessary comparison as I had hoped.
What I had in mind was more like this:
for _, diffPath := range diffs {
if len(includePathsRegexps) > 0 || len(includePathsGlobs) > 0 ||
len(includePathsPrefixes) > 0 {
// If none of the above had been non-empty, the path is implicitly included
matches, err := matchesGlobs(diffPath, includePathsGlobs)
if err != nil {
return false, fmt.Errorf(
"syntax error in include glob patterns: %w",
err,
)
}
if !matches && !matchesPrefix(diffPath, includePathsPrefixes) &&
!matchesRegexpList(diffPath, includePathsRegexps) {
// diffPath didn't match any of the include filters
continue
}
}
// If we get to here, the path was either implicitly or explicitly included
matches, err := matchesGlobs(diffPath, excludePathsGlobs)
if err != nil {
return false, fmt.Errorf(
"syntax error in exclude glob patterns: %w",
err,
)
}
if matches || matchesPrefix(diffPath, excludePathsPrefixes) ||
matchesRegexpList(diffPath, excludePathsRegexps) {
// diffPath matched an exclude filter
continue
}
// If we get to here, the path was not explicitly excluded
return true, nil
}
return false, nilThere was a problem hiding this comment.
Actually... hold off on that... @hiddeco and I are discussing another alternative.
There was a problem hiding this comment.
After some back and forth, @hiddeco and I agreed something along these lines will check both boxes of avoiding unnecessary comparisons and making the code easier to follow overall.
There was a problem hiding this comment.
After some back and forth, @hiddeco and I agreed something along these lines will check both boxes of avoiding unnecessary comparisons and making the code easier to follow overall.
@hiddeco @krancour I will refactor and test it out this week
There was a problem hiding this comment.
Thanks @maksimstankevic. We didn't thoroughly test it. It was a "something like this should work." I did run the existing tests though and saw only minor issues that are probably fixed easily. Didn't go too deep.
There was a problem hiding this comment.
@krancour @hiddeco along these lines worked out of the box, no work for me other than testing, changes pushed. Elegant and scalable design.
Signed-off-by: Maksim Stankevic <maksim.stankevic1@gmail.com>
Signed-off-by: Maksim Stankevic <maksim.stankevic1@gmail.com>
|
@maksimstankevic I think this looks pretty great. Thanks for the hard work. At a later date, when documenting the changes, I may adjust the comments on the LGTM unless @hiddeco has anything more to say about it. (@hiddeco go ahead and merge if you're good.) |
hiddeco
changed the title
hiddeco
changed the title
hiddeco
changed the title
There was a problem hiding this comment.
Also happy with how this looks, thanks @maksimstankevic! 🍒
As another potential follow up, I think we could improve the tests a bit to provide some guarantees when people mix-and-match patterns. But that's absolutely non-blocking for now.
Just filed a new issue for this: #1862, will take it and add thorough testing thanks @hiddeco ! |
…e Git Subscription