Fix pagination with listing self-hosted runners #202
Conversation
|
Thanks for the PR will check asap. |
| return DEFAULT_REGISTERED_RUNNERS; | ||
| }); | ||
| mockOctokit.actions.listSelfHostedRunnersForRepo.mockImplementation(() => { | ||
| mockOctokit.paginate.mockImplementation(() => { |
There was a problem hiding this comment.
Just wondering if we can find a better way to mock the specific function, this will mock all pagnate calls. For now no issue but create potential problems in the future.
There was a problem hiding this comment.
Maybe it is possible to add a check to tests, something like:
expect(mockOcktokit.paginate).toBeCalledWith(mockOctokit.actions.listSelfHostedRunnersForOrg, expect.any());Haven't tested if it works.
There was a problem hiding this comment.
I could scope the mocking of the implementation to each individual test, so that it is restored inbetween each test. Does that work?
There was a problem hiding this comment.
I noticed it's already been scoped to scaleDown, I think it's fine for now unless you're uncomfortable with the current test implementation
There was a problem hiding this comment.
The risk is when we add pagination to more github call they all now handled by the same mock, all calls to mockOctokit.paginate will gets the same mocked response. Wil works as long we not paginate any other call. If would prefer if we can come with a fix. Otherwise I am fine with creating an issue and keep it for the moment. @gertjanmaas can you have a look again?
|
Been testing this PR on our own org, I've discovered that the lambda ends up timing out because if a large amount of runners are returned from the call, we make a lot of requests to github API, I will need to implement a local cache to speed up the lambda execution time. |
How many runners do you have on averagen? |
|
~30 or so |
|
@npalm added function memoization, should reduce execution time by around 75% per my testing |
|
@npalm @gertjanmaas anything i can do here to get this merged in? |
Fixes #227