feat(lambda): move to a single github client with conditional requests#4479
feat(lambda): move to a single github client with conditional requests#4479iainlane wants to merge 1 commit intogithub-aws-runners:mainfrom
Conversation
We're currently creating fresh clients for each lambda invocation. This is not entirely optimal, because we'll potentially request the same data from the API multiple times. Move instead to a single central client. This is created at the module scope and then passed down into the handlers. The client also uses `toad-cache` to cache previously-seen responses and send `If-None-Match` and `If-Modified-Since` headers to the API to allow it to avoid sending us back data we already have in our hands. When this happens we get a "304 Not Modified" response, we use our copy, and the call doesn't consume the rate limit.
|
First of all, thanks for the work and the contribution. Not digged deep yet, but here some first thougts. Feel free to DM my on discord for a chat. There is indeed a lot that can be improved on the use of the github client. I think the first improvement is starting a lib package to proxy calls to GitHub API's. This will make mocking in toher test a lot easier, and avoids code duplication. Indeed caching is not done properly, I think only implemented in the scale-down lambda. By moving GitHub in a lib / service layer we can do this miuch easier and consequent. THe library tyou introduced seems doing the magic, but it is not that well maintained in my point of view. Would prefer to keep the depended libs to a bare minimum or at leas very common and will maintained onces. This to avoid supply-chain attacks and keep the module maintainable. |
|
Cheers, I'm not there with this PR yet - I haven't fixed the tests or anything for example! 😁 I picked Besides sharing the client & caching between invocations, the other benefit will indeed be for devs and testing: all GitHub client code should end up centralised in one place. We can figure out what the right encapsulation is together. 👍 re: discord, I'm already there as |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs. Thank you for your contributions. |
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs. Thank you for your contributions. |
2 participants
We're currently creating fresh clients for each lambda invocation. This is not entirely optimal, because we'll potentially request the same data from the API multiple times.
Move instead to a single central client. This is created at the module scope and then passed down into the handlers. The client also uses
toad-cacheto cache previously-seen responses and sendIf-None-MatchandIf-Modified-Sinceheaders to the API to allow it to avoid sending us back data we already have in our hands. When this happens we get a "304 Not Modified" response, we use our copy, and the call doesn't consume the rate limit.