Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: pagination for ListWorkflowJobs in autoscaler (#990) #992

Merged

Conversation

larhauga
Copy link
Contributor

@larhauga larhauga commented Dec 8, 2021

Adding handling of paginated results when calling ListWorkflowJobs. By default the per_page is 30, which potentially would return 30 queued and 30 in_progress jobs.

This change should enable the autoscaler to scale workflows with more than 60 jobs to the exact number of runners needed.

Problem: I did not find any support for pagination in the Github fake client, and have not been able to test this (as I have not been able to push an image to an environment where I can verify this).
If anyone is able to help out verifying this PR, i would really appreciate it.

Ref #990

@mumoshu mumoshu force-pushed the autoscaling-workflowjobs-pagination branch from 3dcf52b to 180e645 Compare December 14, 2021 09:04
@mumoshu
Copy link
Collaborator

mumoshu commented Dec 14, 2021

@larhauga Thanks for your contribution! Rebased and I'm now trying to test this locally. If you could help double-checking, it would be great if you could follow our contribution guide for local testing.

@@ -10,6 +10,7 @@ import (
"time"

"github.com/actions-runner-controller/actions-runner-controller/api/v1alpha1"
"github.com/google/go-github/v37/github"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"github.com/google/go-github/v37/github"
"github.com/google/go-github/v39/github"

This seems outdated since #925. I'll update your commit with the change.

@mumoshu mumoshu force-pushed the autoscaling-workflowjobs-pagination branch from 180e645 to e3c3c39 Compare December 14, 2021 09:09
@mumoshu mumoshu added this to the v0.21.0 milestone Dec 15, 2021
@mumoshu
Copy link
Collaborator

mumoshu commented Dec 24, 2021

Sry it took much longer time than I had thought to have time to test this manually! Code LGTM so let merge this anyway and make follow-ups only after we find any issues layer.
Thanks for your contribution @larhauga ☺️

@mumoshu mumoshu merged commit c5950d7 into actions:master Dec 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants