LukePrior / nbn-upgrade-map

Interactive map showing premises eligible for the NBN FTTP upgrade program.
https://nbn.lukeprior.com/
MIT License
126 stars 11 forks source link

Github commit API rate limit #309

Open LukePrior opened 10 months ago

LukePrior commented 10 months ago

Just got this, should be error handled in frontend

{"message":"API rate limit exceeded for XXX. (But here's the good news: Authenticated requests get a higher rate limit. Check out the documentation for more details.)","documentation_url":"https://docs.github.com/rest/overview/resources-in-the-rest-api#rate-limiting"}

lyricnz commented 10 months ago

Is that from fetching the git commit history?

LukePrior commented 10 months ago

Yes

lyricnz commented 10 months ago

Should we cache the log into file(s) within our repo, so it doesn't need to hit the real API?

LukePrior commented 10 months ago

Yeah might need to do that

LukePrior commented 10 months ago

Or rmaybe site can just be modified as it seems to reload every time even if the only change is the selected commit, should only reload when doing a new suburb.

lyricnz commented 10 months ago

Doesn't your new app-caching thing do that?

LukePrior commented 10 months ago

Doesn't your new app-caching thing do that?

Not in its current state as I don't know how long it can safely be cached for because it might get updated in a few hours all the way up to 17 days.

lyricnz commented 10 months ago

I have some code that generates a changelog for each result file. Not sure it's a good idea: either adding 15,000 files to git, or putting them per-state (which are big).

Plus we need to maintain the file, which naturally causes more commits. Maybe we can include "last run changes" at the start of next cycle...

lyricnz commented 10 months ago

Not in its current state as I don't know how long it can safely be cached for because it might get updated in a few hours all the way up to 17 days.

Technically, a client can know whether it needs to refresh the commit-log cache based on the state processed_date in results/combined-suburbs.json vs the date of the newest commit in the cache (for a particular suburb).

lyricnz commented 10 months ago

I'll draft a PR that creates an index file (as small as possible), and uses it from the frontend. If that looks OK, we can add process to update the index, cache it from the client, etc.

Will use short-hashes, and unixtime, and truncated filenames to minimise size.

lyricnz commented 10 months ago

Added a call to update the index at the start of processing - this will add any commits that it's missing. Can't do it at the end (and include this-cycle commits) because we don't know the commit hash yet, and otherwise would need two commits (ugly).

LukePrior commented 10 months ago

I can try updating the service worker to read the date of the latest commit and set it not to expire for 2 weeks from that date or if 2 weeks has already elapsed perhaps 1 day.

lyricnz commented 10 months ago

Since the git-commit-log of all the suburbs are in the same file now, I think just caching it for a day would be fine (see the PR). https://github.com/LukePrior/nbn-upgrade-map/pull/317

lyricnz commented 9 months ago

@LukePrior thoughts on PR?