New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: no_changed_files predicate #756
base: develop
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates. I think the SkipPhrase
makes sense but we'll need to either provide a default value or set "do not"
as the value in all existing predicates to avoid breaking the details display for non-file predicates.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good - I've changed this to "match" in both scenarios with the reworked use of skip phrase.
Summary
Adds a new
no_changed_files
predicate that allows you to ensure that the rule is not applied if a file matching an entry inno_changed_files.paths
is present.This is a negation of the
changed_files
predicate, and the implementation logic reflects that.Closes #754
Related #755