policy-bot: Policy bot stuck on `Commit hash does not have a pushed date`

This should now be available as docker.io/palantirtechnologies/policy-bot:snapshot if anyone is interested in trying it out. I think we should leave this issue open to continue to track further improvements and attempt to correctly invalidate commits pushed after approvals.

From github support, it looks like this behaviour won’t land in GHES until 3.11, which is at least one more major version away (likely in 2024 based on the timing of previous releases).

Thanks for the report. Its definitely a shame that the field has been removed from the graphql api:

Breaking A change will be made to Commit.pushedDate.
Description:
pushedDate will be removed.

Reason:
pushedDate is no longer supported.

You are correct @zliu2-wish that the PushedDate is used to determine approvals with the invalidate_on_push option, so this breaking change is particularly problematic.

A permanent fix for this issue is now available in version 1.31.0. If you previously removed the invalidate_on_push option from your policy files, it should be safe to re-add them after upgrading to this version. While you can continue to the set the do_not_load_commit_pushed_date option, it’s ignored by the server and has no effect.

If you encounter any issues with invalidate_on_push as a result of the new implementation, please file a new issue and we’ll take a look.

Unfortunately, the way Policy Bot evaluation currently works requires being able to order commits and comments at any time, which means we can’t easily switch to invalidating based on events without a larger refactor.

I do think that’s how we’ll have to fix this, but I’ll have to think a bit about how to best do it. I think we’ll have to store state somehow, but I’d really like to avoid introducing an external database. I think we can do something by looking at the times of previous statuses posted by Policy Bot, but I’m not clear on all the details yet.

Thank you @asvoboda. The patch and ENV work with invalidate_on_push disabled. To skip the action for those repositories, I have forked and make a simple workaround removing the r.Options.InvalidateOnPush function PR The image is available at https://hub.docker.com/r/waynechowhk01/policy-bot/tags . Feel free to use it with docker pull waynechowhk01/policy-bot:snapshot

According to this, a breaking change was made to Commit.pushedDate and pushedDate appears to be no longer supported.

@asvoboda

Update: adding the env variable (POLICYBOT_OPTIONS_DO_NOT_LOAD_COMMIT_PUSHED_DATE: true) makes it work for the repo that had invalidate_on_push fully removed from .policy.yml

However, for repos with invalidate_on_push still present, am getting this error now:

failed to filter candidates: no commit contained a push date

Going to go through the repos and remove the references one by one

Much appreciated for the fix

I think we’ll want to create an option to disable the push date loading for commits that can be toggled at the server level. Right now we attempt to load the pushed dates for commits even for rules that don’t use the invalidate_on_push option, which is contributing to slowness in requests.

We are hitting the issue as well. Don’t have context into the repo but thanks to @ab77 did some digging in the code and found PushedDate being used: https://github.com/palantir/policy-bot/blob/51b4afb12257e1a3507c534cf91d2860d606296f/pull/github.go#L925

Looking at the graphql docs on Commit that field did get deprecated: https://docs.github.com/en/graphql/reference/objects#commit

I went through all the fields looking for possible replacements and found these fields: authoredDate committedDate

Could consider swapping out the pushDate for the above two fields but one possible issue I see:

• if the pushDate was used to determine when to reset approvals, using the other two fields allows someone to author/commit early but hold the change locally
• The user would be able to submit v1 of the changes (but already have v2 locally). Get the approval for v1 then push out v2. Since the v2 author/commit time was before the approval, it wouldn’t reset the approvals

I’d like to run one more test pass against our internal staging environment to verify everything before release. I’m planning to do that today, so hopefully I can tag a release by the end of the day (PDT).

More information from support:

We discovered that the Commit GraphQL object returns a PushedDate field. This field is calculated in a problematic way. We do not have a way to tie a commit back to a specific push, so we cannot reliably calculate the first time we see a commit and which push it belongs to.

Instead, this field attempts to do it by finding the first Push in the pushes table across all repositories that has an after SHA that matches the commit in question. This doesn’t work if the commit is somewhere in the middle of a group of commits in a push. It’s possible that the commit’s SHA isn’t ever a push’s after SHA. It’s also possible that we do find a push with the matching after SHA, but it’s not the first time we saw the commit.

All this to say, the field is very unreliable, and not accurate.

This probably explains why policy-bot needs a significant amount of code to handle pushedDate unreliability in https://github.com/palantir/policy-bot/blob/develop/pull/github.go#L856-L875

Thats correct; it looks like on github.com support for invalidate on push has been entirely broken.

From support:

The Commit.pushedDate field was deprecated because the information calculated and presented is incorrect. The noticed was first announced in our changelog here and then deprecation happen 2023-07-01 as you already noted.

As I understand, there is currently no way to map a commit to a push, and establish that it’s the first time the commit has been seen. There isn’t a similar date object elsewhere in the API that can replace this.

We hope one day to offer some mapping from commit to push as it is on our radar but this isn’t something that will be prioritized in the short/medium term.

I’m going to file a ticket with our enterprise reps to better understand what options we have on the GH side (#2241100).

I believe this should only affect policies where invalidate_on_push is used. I think the only short term option at the moment is to write a policy that has a fallback OR condition where a maintainer/admin has approved. I’ll continue thinking about this.