feat: no_changed_files predicate #756
Conversation
|
Thanks for your interest in palantir/policy-bot, @erikburt! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
|
Thanks for implementing this. I think it looks good, but I need to think a bit more about:
I'll try to get back to you on those points in the next few days. |
There was a problem hiding this comment.
I think this is good in terms of the logic: a no_changed_files predicate should be the exact opposite of a changed_files predicate. But I had some suggestions for improving how this displays in the details UI.
I'm also thinking that the logic for the metadata values (description, conditions, etc.) might be clearer if the predicate was implemented directly, instead of by modifying the results of the ChangedFiles predicate. Most of the code in both predicates is for setting up this metadata, so we're not saving all that much code by delegating to the other predicate.
erikburt
force-pushed
the
feat/no-changed-files
branch
from
fc2a1ef
to
9ba9cdd
Compare
|
Thank you for the review! Sorry for the delay, was caught up with some deadlines. I've changed the phrases around using some of your suggestions. To get around the weird double negatives I added a "SkipPhrase" to the predicate result which is universally "do not" except for the NoChangedFiles predicate where it is empty. I've also tried to simplify the logic where possible. I do think it's probably best to reuse the ChangedFiles predicate to ensure this is a direct negation. If you would like me to duplicate and reverse the logic, I can do that as well. Let me know what you think. |
server/templates/details.html.tmpl
Outdated
| @@ -158,7 +158,7 @@ | |||
| <ul class="list-disc list-outside pl-6 py-2"> | |||
| {{range .Values}}<li class="font-mono text-sm-mono">{{.}}</li>{{end}} | |||
| </ul> | |||
| {{if eq $s "skipped"}}do not {{end}}{{.ConditionPhrase}} | |||
| {{if eq $s "skipped"}}{{.SkipPhrase}} {{end}}{{.ConditionPhrase}} | |||
There was a problem hiding this comment.
One issue with using SkipPhrase like this is that we'll need to either set a value in all of the existing predicates or provide a way to use a default value when it isn't set.
There was a problem hiding this comment.
I've modified the code so a skip phrase is not directly needed. The logic now allows you to reverse when to add the "do not", defaulting to current behaviour for all predicates.
policy/predicate/file.go
Outdated
| predicateResult.ConditionPhrase = "do not match" | ||
| predicateResult.Description = "No changed files match the specified patterns" | ||
| } else { | ||
| predicateResult.ConditionPhrase = "should not match" |
There was a problem hiding this comment.
I think this may be better as "match". That way, the overall expression will read This rule is skipped because the changed files [...] match the path patterns [...]
There was a problem hiding this comment.
Sounds good - I've changed this to "match" in both scenarios with the reworked use of skip phrase.
Summary
Adds a new
no_changed_filespredicate that allows you to ensure that the rule is not applied if a file matching an entry inno_changed_files.pathsis present.This is a negation of the
changed_filespredicate, and the implementation logic reflects that.Closes #754
Related #755