Implement invalidate_on_push without pushedDate field #602

bluekeyes · 2023-07-10T03:42:55Z

This is a draft PR to explore how to implement the invalidate_on_push option without using the pushedDate field that has been removed from the GraphQL API. The basic idea is to use the timestamps of past policy-bot statuses to figure out which comments/reviews/etc. are relevant for a particular evaluation:

If no commits have statuses, it's either the first evaluation or a force push replaced all commits, so ignore anything older than the current time (which needs to be the same for all rule evaluations in a policy)
If some commits have statuses, it's a new push to an existing PR, so ignore anything older than the most recent status timestamp
If all commits have statuses:
1. If there's only one timestamp, it's a re-evaluation of a PR with a single push, so consider everything
2. If there's more than one timestamp, it's a re-evaluation of a PR with multiple pushes, so ignore anything older than the second most recent status timestamp

Right now, I've only changed the pull.Context and the logic described above, so it doesn't compile. I still need to finish connecting all the parts and then make sure everything works.

Closes #598.

First step in removing our use of the deleted pushedDate field to implement invalidate_on_push. Load the last policy-bot status (if it exists) to approximate the evaluation time instead.

bluekeyes · 2023-07-12T17:01:34Z

It looks like this works in most situations, but one problem case is if a force-push replaces only some of the commits in the PR. Right now, that moves the evaluation cutoff to the time of the last remaining commit, which can lead to incorrect approval. I'm not sure how to solve this yet, since the force-push removed the state that we'd otherwise use to detect the push.

It looks like force-pushes are recorded in the pull request timeline, but we had some historic issues with that API not returning information fast enough and leading to incorrect results (#47, #54), so I'm a bit hesitant to use it again.

bluekeyes · 2023-07-14T03:23:51Z

After running in to various edge cases that didn't work in the previous approach, I switched to listing the statuses on the (filtered) head commit and using the creation time of the oldest status as the push time. For the most part this is simpler although it does require new API calls. The new calls can be cached and I think are balanced by the removal of all the pushedDate fallback calls.

This method leaves a bit of a gap during webhook delivery and processing where an approval could be incorrectly ignored, but I think this is acceptable and is better than incorrectly approving a PR that should require a new approval. This should only be an issue if either processing is very delayed or a user leaves an approval within seconds of a new push.

I need to test the interactions with ignored commits a bit more, but otherwise, I think this is working.

bluekeyes · 2023-07-14T03:24:50Z

policy/approval/approve.go

@@ -301,10 +301,6 @@ func (r *Rule) filterEditedCandidates(ctx context.Context, prctx pull.Context, c
 func (r *Rule) filterInvalidCandidates(ctx context.Context, prctx pull.Context, candidates []*common.Candidate) ([]*common.Candidate, []*common.Dismissal, error) {
 	log := zerolog.Ctx(ctx)

-	if !r.Options.InvalidateOnPush {


This is already checked by the caller

bluekeyes · 2023-07-14T03:25:13Z

README.md

@@ -31,7 +31,6 @@ UI to view the detailed approval status of any pull request.
    - [`or`, `and`, and `if` (Rule Predicates)](#or-and-and-if-rule-predicates)
    - [Cross-organization Membership Tests](#cross-organization-membership-tests)
    - [Update Merges](#update-merges)
-    - [Private Repositories](#private-repositories)


This section was removed a long time ago, but we didn't update the ToC

it's not still necessary to maintain a ToC by hand, at least for the github web UI: https://github.blog/changelog/2021-04-13-table-of-contents-support-in-markdown-files/

bluekeyes · 2023-07-14T03:26:44Z

policy/approval/approve.go

+
+// sortCommits orders commits in history order starting from head. It must be
+// called on the unfiltered set of commits.
+func sortCommits(commits []*pull.Commit, head string) []*pull.Commit {


I think this is unnecessary because GitHub already returns the commits in this order, but I'd rather be explicit than have this break strangely if the order changes for some reason - this ordering is important for push detection to work in all cases.

Is it worth adding some small test cases for this ordering function?

bluekeyes · 2023-07-14T03:44:14Z

pull/github.go

 	membership    map[string]bool
 	statuses      map[string]string
 	labels        []string
+	pushedAt      map[string]time.Time


This could actually be cached globally: once we know the push time for a specific commit in a specific repository, it shouldn't change. I think for now it's easiest to re-compute it on each request, but if the extra requests become a problem, this might be a good optimization.

DepickereSven · 2023-07-14T11:45:29Z

server/handler/eval_options.go

@@ -84,12 +83,3 @@ func setStringFromEnv(key, prefix string, value *string) bool {
 	}
 	return false
 }
-
-func setBoolFromEnv(key, prefix string, value *bool) bool {


Isn't removing the support for global env also not a breaking change?

With this change, we never attempt to load the pushedDate field, so the do_not_load_pushed_date option has no effect. In the YAML configuration, the parser errors if the file contains unknown fields. But for environment variables, there's no issue with setting values that Policy Bot doesn't know about, so I think this should be fine.

Thank you for explaining @bluekeyes

asvoboda

The idea to use past statuses on commits seems like a really good workaround for the missing data problem. Its a bit annoying that we've lost data from GH, but I think this should work in most cases. I'm a bit worried about some race between the few seconds after a push and a new status showing up, but as you mentioned in practice I think this probably won't be an issue.

asvoboda · 2023-07-17T15:56:28Z

policy/approval/approve.go

+
+// sortCommits orders commits in history order starting from head. It must be
+// called on the unfiltered set of commits.
+func sortCommits(commits []*pull.Commit, head string) []*pull.Commit {


Is it worth adding some small test cases for this ordering function?

asvoboda · 2023-07-23T22:18:39Z

👍 LGTM

bluekeyes · 2023-07-24T00:30:15Z

#612 adds a global cache for this data, since I'm worried about the cost of all the API calls to list statuses, particularly as a commit accumulates more statuses over time. I'll merge that separately, since this PR is already making enough changes.

See: palantir/policy-bot#602 See: product-os/policy-bot#4 See: Change-type: patch Signed-off-by: Kyle Harding <kyle@balena.io>

See: palantir/policy-bot#602 See: product-os/policy-bot#4 See: product-os/environment-production#623 Change-type: patch Signed-off-by: Kyle Harding <kyle@balena.io>

bluekeyes added 6 commits July 9, 2023 23:31

Replace PushedAt with LastEvaluatedAt

f307fec

First step in removing our use of the deleted pushedDate field to implement invalidate_on_push. Load the last policy-bot status (if it exists) to approximate the evaluation time instead.

Implement evaluation cutoff logic

2ec5748

Fix non-test compilation errors

bfaf1a1

Fix existing tests

7ab68cd

Use correct status check vaues in queries

40fdd84

Add tests for new invalidation cases

56fb7fa

bluekeyes added 4 commits July 12, 2023 18:14

Always use current time for new commits

56ee0c4

Update README

3f3e779

Use first status timestamp as push time

a39a23a

Account for ignored commits

4a92d55

bluekeyes marked this pull request as ready for review July 14, 2023 03:23

bluekeyes commented Jul 14, 2023

View reviewed changes

bluekeyes requested a review from asvoboda July 14, 2023 03:27

bluekeyes commented Jul 14, 2023

View reviewed changes

DepickereSven reviewed Jul 14, 2023

View reviewed changes

asvoboda approved these changes Jul 17, 2023

View reviewed changes

Add tests for commit sorting

4199679

bluekeyes merged commit efde8be into develop Jul 24, 2023

bluekeyes deleted the bkeyes/remove-pushed-date branch July 24, 2023 21:42

klutchell added a commit to product-os/policies that referenced this pull request Jul 31, 2023

Re-enable invalidate on push for external contributions

0462501

See: palantir/policy-bot#602 See: product-os/policy-bot#4 See: Change-type: patch Signed-off-by: Kyle Harding <kyle@balena.io>

klutchell mentioned this pull request Jul 31, 2023

Re-enable invalidate on push for external contributions product-os/policies#45

Merged

bluekeyes mentioned this pull request Oct 20, 2023

Certain merges can lead to ignored commits during evaluation #647

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement invalidate_on_push without pushedDate field #602

Implement invalidate_on_push without pushedDate field #602

bluekeyes commented Jul 10, 2023 •

edited

Loading

bluekeyes commented Jul 12, 2023 •

edited

Loading

bluekeyes commented Jul 14, 2023

bluekeyes Jul 14, 2023

bluekeyes Jul 14, 2023

DannyGoldberg Jul 20, 2023

bluekeyes Jul 14, 2023

asvoboda Jul 17, 2023

bluekeyes Jul 14, 2023

DepickereSven Jul 14, 2023

bluekeyes Jul 14, 2023

DepickereSven Jul 14, 2023

asvoboda left a comment

asvoboda Jul 17, 2023

asvoboda commented Jul 23, 2023

bluekeyes commented Jul 24, 2023

Implement invalidate_on_push without pushedDate field #602

Implement invalidate_on_push without pushedDate field #602

Conversation

bluekeyes commented Jul 10, 2023 • edited Loading

bluekeyes commented Jul 12, 2023 • edited Loading

bluekeyes commented Jul 14, 2023

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

asvoboda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asvoboda commented Jul 23, 2023

bluekeyes commented Jul 24, 2023

bluekeyes commented Jul 10, 2023 •

edited

Loading

bluekeyes commented Jul 12, 2023 •

edited

Loading