feat(github): use REST for etag caching of issues

[rarkins]

Changes

Refactor getIssues() to use github REST API and benefit from etag caching

Context

Documentation (please check one with an [x])

• I have updated the documentation, or
• No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

• Code inspection only, or
• Newly added/modified unit tests, or
• No unit tests but ran on a real repository, or
• Both unit tests + ran on a real repository

[rarkins]

Merge #26793 first

[rarkins]

I need to think what the effect is if there's multiple pages. Maybe should cache the entire list. Also time to check page size and count again

[zharinov]

Seems like etags for issues/PR lists are very unstable: https://twitter.com/CAFxX/status/1490560328261791746
Example:

• Query issue list on some small repo
• Go to the Pull Requests and add comment
• Update issue list and observe different etag

This is why we ended up with dropping them for cached PR list.

[rarkins]

Now I remember that. Can you use the If-Modified-Since approach instead?

[zharinov]

I've got 304 only 2 times out of 20 when querying test repo with If-Modified-Since with value from the Date header of the previous one. So seems like it doesn't work too.

[rarkins]

So let's go with etag for now but add an issue to investigate modified date? I thought I for a reasonable amount of success with 304s

[zharinov]

If-Modified-Since gives 18/20 false cache resets for repository with zero activity. Basically, seems like if we rely on how ETag and If-Modified-Since work for GitHub, we could just disable any caching.

[rarkins]

Maybe it's particularly bad for the lists of issues and PRs?

[rarkins]

https://docs.github.com/en/rest/using-the-rest-api/best-practices-for-using-the-rest-api?apiVersion=2022-11-28#use-conditional-requests-if-appropriate

https://jamiemagee.co.uk/blog/making-the-most-of-github-rate-limits/ :)

[zharinov]

I think there could be another approach: thanks to repoInit(), we're doing at least one GraphQL request anyway, right? We could pre-fetch some small, but reasonable amount of issues and PRs with this init query, update our caches if needed, and then decide whether to perform further sync or we're good.

[rarkins]

Is there any downside to switching from graohql to rest here though? Even if the tag frequently doesn't work, maybe 20-30% it does

[rarkins]

@zharinov It's still TBD whether to switch initRepo to REST too :) can we do the same issue/PR filtering in that unit repo query?

[zharinov]

Well, I didn't remember we already switched to REST for PRs, so maybe yes and probably we could just fetch all from /repos/:owner/:repo/issues endpoint.

Speaking of cache hit rate, we could count it, though my bet is 5-10%

[viceice]

otherwise LGTM

[renovate-release]

🎉 This PR is included in version 37.219.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)
• 37.219.0

Your semantic-release bot 📦🚀

[mtardy]

Hey, this morning, 37.219.0 ran on my repository and I think this is the reason why Renovate didn't find its dashboard issue and recreated a new one.

Yesterday evening I got:

DEBUG: findIssue(Dependency Dashboard) (repository=cilium/tetragon)
DEBUG: Retrieving issueList (repository=cilium/tetragon)
DEBUG: Retrieved 1 issues (repository=cilium/tetragon)
DEBUG: Found issue 1054 (repository=cilium/tetragon)
DEBUG: http cache: saving https://api.github.com/repos/cilium/tetragon/issues/1054 (etag=W/"4f3474e44487e1c736d399d19e372a5f6d8280f1e8e733bbbc2e4a46666820d3", lastModified=Tue, 27 Feb 2024 17:08:41 GMT) (repository=cilium/tetragon)

This morning:

DEBUG: findIssue(Dependency Dashboard) (repository=cilium/tetragon)
DEBUG: Retrieving issueList (repository=cilium/tetragon)
DEBUG: http cache: saving https://api.github.com/repos/cilium/tetragon/issues?creator=cilium-renovate[bot]&state=all (etag=W/"f6c20b2f8fe7c51530b52293ea1fb943ee97c3d570f3310591903b76fb27c79f", lastModified=undefined) (repository=cilium/tetragon)
DEBUG: Retrieved 300 issues (repository=cilium/tetragon)

So it recreated a new issue and now it seems the original one is not closed but abandoned (because it doesn't see it):

• original one Dependency Dashboard cilium/tetragon#1054
• new one Dependency Dashboard cilium/tetragon#2159

I'm not sure what will happen if I rollback and keep both dashboards.

[mtardy]

sadly the rest API returns pulls as issues 😔 so I don't think this will really help as @zharinov already said.

maybe hide the change behind a feature flag, so the app can test it on some orgs first?

A colleague of mine mentioned that it might be the issue:

graphql -> listing issues only
rest api -> lists issues and PRs

That + a pagination issue might explain why it could not find its own "old" dashboard.