snok / container-retention-policy

GitHub action for pruning old GHCR container image versions.
MIT License
186 stars 30 forks source link

Unhandled exception raised at runtime: `'x-ratelimit-remaining'`. #57

Closed id-tari closed 4 months ago

id-tari commented 1 year ago

Running action v2. During the first run it has discovered and deleted 147 untagged, 1 week old images. By the end action throws exception messages:

Unhandled exception raised at runtime: `'x-ratelimit-remaining'`. Please report this at https://github.com/snok/container-retention-policy/issues/new

604 times/log-lines

Unfortunately there are no additional outputs, and action did not exit with error. Action has finished "successfully".

Initially as test I was trying to delete only untagged, at least 1 week old images. After initial run I had left 521 untagged image.

2nd run has discovered 70 images for deletion. Tons of exception messages still appear. 3rd run deleted 96 images. 4th run deleted 61 images. etc ...

Even "enable debug output" on action run is not showing anything useful. I'm not sure how to debug this behavior, will greatly appreciate your ideas. (not a pro python developer)

sondrelg commented 1 year ago

Thanks @id-tari, the error you see is the "catch-all" exception handler here :+1:

If I had to guess, I'm assuming the response from github doesn't contain the x-ratelimit-remaining header we're expecting here. We should probably change this code to:

-    if int(response.headers['x-ratelimit-remaining']) == 0:
+    if int(response.headers.get('x-ratelimit-remaining', default=0)) == 0:

That's just my first instinct - could be wrong :slightly_smiling_face:

Would you be interested in trying to create a fix? If you're comfortable with it, how you would need to do it is:

  1. Fork this project
  2. Commit the change above
  3. Then in your workflow, to test it works, you can change - uses: snok/container-retention-policy@v2 to - uses: id-tari/container-retention-policy@main and run the workflow, and it should run it with your new code
  4. Submit the PR here with the fix :slightly_smiling_face:

If that sounds too complicated, I might have time to fix this later in the week :slightly_smiling_face:

id-tari commented 1 year ago

Thanks @sondrelg! I've applied and tried your suggested fix.

Now there is another error:

Unhandled exception raised at runtime: 'x-ratelimit-reset'.

This time it does not make sense to set default value for a key if header is not found.

Something is wrong really wrong, headers and values are missing.

Unfortunately looks like I don't have time to learn asyncio and httpx library. I hope this will be a trivial issue to fix for somehow who has implemented that.

Docs: https://docs.github.com/en/rest/rate-limit?apiVersion=2022-11-28 https://docs.github.com/en/rest/overview/resources-in-the-rest-api?apiVersion=2022-11-28#rate-limit-http-headers

sondrelg commented 1 year ago

Sorry, actually if you just do this instead

-    if int(response.headers['x-ratelimit-remaining']) == 0:
+    if int(response.headers.get('x-ratelimit-remaining', default=1)) == 0:

it should work

id-tari commented 1 year ago

Yay worked! I'll make a PR from a different acc.

Pho3niX90 commented 1 year ago

Same same. just posting here so there are no duplications. image

sondrelg commented 1 year ago

Thanks @Pho3niX90. I must have missed the PR for this. If I can I'll take a look and get it merged this weekend :+1:

sondrelg commented 4 months ago

The latest release redoes rate limiting completely. It should fix this issue, and a few more.

If you have time, the migration guide for v3 is included in the release post 👍 I would greatly appreciate feedback if you have any.

If you run into any issues, please share them in the issue opened for tracking the v3 release ☺️