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

[joseparajelesGL]

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

[asvoboda]

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.

[asvoboda]

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).

[bluekeyes]

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.

[bluekeyes]

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.

[ab77]

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

[waynechowhk01]

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

[zliu2-wish]

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:

policy-bot/pull/github.go

Line 925 in 51b4afb

PushedDate *time.Time

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

[asvoboda]

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.

[zliu2-wish]

@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

[asvoboda]

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.

[asvoboda]

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.

[asvoboda]

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

[asvoboda]

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

[bluekeyes]

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).

[esmelser-apex]

We are also experiencing this issue and would love to see a fix. ➕ 1️⃣

[johnwonkim]

+1 as well.

[DepickereSven]

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.

So this would mean that for now there is no solution in the near future to be able to use invalidate_on_push?

[DepickereSven]

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

That's sad to hear because that was powerful in combination with the property ignore_commits_by.
So now when we switch back to letting GitHub decide to invalidate on push that property will no longer work either.

[samandmoore]

Wow. Well this is quite unfortunate. It's especially weird because GitHub seems to support similar behavior with their "dismiss stale reviews" branch protection rule setting. It clearly dismisses reviews older than the latest pushed changes on a PR.

[lamebear]

Could policy bot be updated so that if:

1. (github.PullRequestEvent).GetAction() == "synchronize", and
2. github.PullRequestEvent.PullRequest.Head.SHA has changed

it invalidates on push?

[agirlnamedsophia]

@asvoboda this appears to resolve our issues! thank you.

[joseparajelesGL]

Can confirm as well, latest snapshot version works!
Thank you so much @asvoboda

[zliu2-wish]

Thanks @asvoboda!
Quick question - I have the below config option (following the sample setup) with the latest snapshot and am seeing this error:
failed to compute approval status: failed to list commits: Commit 0403ce40f4 does not have a pushed date. This is usually caused by delays in the GitHub API

I've removed all references to invalidate_on_push in the .policy.yml file. Am I missing something in the config?

options:
  # The path within repositories to find the policy.yml file
  policy_path: .policy.yml
  # The context for status checks created by the bot
  status_check_context: policy-bot
  # The name of the application as registered with GitHub
  app_name: policy-bot

  # As of 2023-07-01 the commit.pushedDate graphql field is removed from GitHub.
  # Setting this option effectively breaks all usage of the invalidate_on_push approval rule
  do_not_load_commit_pushed_date: true

[asvoboda]

@zliu2-wish double check you're using the latest version? https://hub.docker.com/layers/palantirtechnologies/policy-bot/snapshot/images/sha256-4ebb3c982f8601aceaf78886f956ca95e04c0cf37d5a1dd0ec0a346b46049fe9?context=explore

[zliu2-wish]

root@policy-bot-0ac779999e81f3372:~# docker container ls
CONTAINER ID        IMAGE                                      COMMAND                  CREATED             STATUS                 PORTS               NAMES
2847c13056ed        palantirtechnologies/policy-bot:snapshot   "bin/linux-amd64/pol…"   2 hours ago         Up 2 hours                                 docker_policy-bot

root@policy-bot-0ac779999e81f3372:/# docker image ls
REPOSITORY                        TAG                 IMAGE ID            CREATED             SIZE
palantirtechnologies/policy-bot   snapshot            4f2e6a7e0ca4        3 hours ago         36.6MB
palantirtechnologies/policy-bot   1.25.0              5d5e389db35d        14 months ago       16.9MB
root@policy-bot-0ac779999e81f3372:/# docker pull palantirtechnologies/policy-bot:snapshot
snapshot: Pulling from palantirtechnologies/policy-bot
Digest: sha256:4ebb3c982f8601aceaf78886f956ca95e04c0cf37d5a1dd0ec0a346b46049fe9
Status: Image is up to date for palantirtechnologies/policy-bot:snapshot

Will try to see if using the env variable option would help

[sawyerward]

@asvoboda It's working for us as well. Thank you.

[RyanDinglei]

works for me. Many thanks.

[gpadavala]

i just tried the snapshot tag "palantirtechnologies/policy-bot:snapshot" with digest "sha256:5e14d46e0bf2ea178f11c6d0b9668131ac9644e88229b25ea68c13f7f087a68e"

I still have the same error

EDIT: seems will have to disable the invalidate_on_push, we have 100's of repositories, so not convenient to change them all though

[rallydan]

@bluekeyes, thanks for this fix (and the caching optimization in #612). Is there some testing or other development still pending, or would it be possible to get a release tag with these changes?