-
Notifications
You must be signed in to change notification settings - Fork 103
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
Policy bot stuck on Commit hash does not have a pushed date
#598
Policy bot stuck on Commit hash does not have a pushed date
#598
Comments
According to this, a breaking change was made to |
We are also experiencing this issue and would love to see a fix. ➕ 1️⃣ |
+1 as well. |
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: Line 925 in 51b4afb
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: Could consider swapping out the pushDate for the above two fields but one possible issue I see:
|
Thanks for the report. Its definitely a shame that the field has been removed from the graphql api:
You are correct @zliu2-wish that the |
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 |
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. |
From support:
|
So this would mean that for now there is no solution in the near future to be able to use |
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 |
More information from support:
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 |
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. |
Could policy bot be updated so that if:
it invalidates on push? |
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. |
This should now be available as 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). |
@asvoboda this appears to resolve our issues! thank you. |
Can confirm as well, latest snapshot version works! |
Thanks @asvoboda! I've removed all references to invalidate_on_push in the .policy.yml file. Am I missing something in the config?
|
@zliu2-wish double check you're using the latest version? https://hub.docker.com/layers/palantirtechnologies/policy-bot/snapshot/images/sha256-4ebb3c982f8601aceaf78886f956ca95e04c0cf37d5a1dd0ec0a346b46049fe9?context=explore |
Will try to see if using the env variable option would help |
@asvoboda It's working for us as well. Thank you. |
Update: adding the env variable ( However, for repos with invalidate_on_push still present, am getting this error now:
Going to go through the repos and remove the references one by one Much appreciated for the fix |
works for me. Many thanks. |
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 |
Thank you @asvoboda. The patch and ENV work with |
This is to resolve an isue with policybot:latest using a field that was removed from Github's API. See palantir/policy-bot#598 for reference
@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? |
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). |
A permanent fix for this issue is now available in version 1.31.0. If you previously removed the If you encounter any issues with |
Resolves: palantir/policy-bot#598 Change-type: patch
Hello, we are seeing errors on policy bot where it's stuck on trying to get the
![image](https://private-user-images.githubusercontent.com/74798317/251599867-86d17c26-3d38-4c12-a00c-3fa196d5526a.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjQ0NjI2MjYsIm5iZiI6MTcyNDQ2MjMyNiwicGF0aCI6Ii83NDc5ODMxNy8yNTE1OTk4NjctODZkMTdjMjYtM2QzOC00YzEyLWEwMGMtM2ZhMTk2ZDU1MjZhLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MjQlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODI0VDAxMTg0NlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPWY4MWEzNWEyY2I0NmY1N2JmMWExNTAzZDc4ZGJiYWUzZmViNjE1Y2RlNDIzZjNkOGFjNWM4MmNlYTNhZjFmMmEmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.mHFmdhplbW2KMLosvaSzTIXOp3JCEj805uPmyBcUByY)
pushed date
Is there anything we can do to debug this issue?
Attached is the stack trace
The text was updated successfully, but these errors were encountered: