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 broken pagination in ActionsWorkflowRuns #1488

Merged
merged 9 commits into from
Oct 11, 2022
Merged

Conversation

luke-engle
Copy link
Contributor

Pagination was broken due to the data in the API response containing a hash instead of an array. This change adds a block to concatenate the workflow_runs array within the hash of each page's data

Pagination was broken due to the data in the API response
containing a hash instead of an array. This change adds a block to
concatenate the `workflow_runs` array within the hash of each page's
data
@luke-engle
Copy link
Contributor Author

I believe there are still other methods that have pagination broken in the same way:

Essentially anything that paginates and has a response that returns a hash instead of an array.

I don't have the time to fix and create tests for the others right away but I'll add it to my todo list and try to fix them within the next couple days (I may be able to find time tomorrow to get this done since it shouldn't take too long). I'm going to convert this PR to a draft until I address those issues as well that way we don't need to create multiple releases to address this

@luke-engle luke-engle marked this pull request as draft September 23, 2022 04:38
@timrogers
Copy link
Contributor

Thanks for picking this up ❤️ As you say, it would be great to fix a bunch of these in one PR if we can.

This is making me wonder if we need to do something in the OpenAPI spec itself to describe how data is paginated.

@luke-engle luke-engle marked this pull request as ready for review October 1, 2022 05:06
@luke-engle
Copy link
Contributor Author

Thanks for picking this up ❤️ As you say, it would be great to fix a bunch of these in one PR if we can.

This is making me wonder if we need to do something in the OpenAPI spec itself to describe how data is paginated.

Ya it seems like the Actions team (who came from Microsoft) are the ones who implemented the the 'total count', which is why pagination is broken in the first place. If it were up to me, I would say to make all the APIs consistent in what they return. In other words, don't return the total count, just return the data, then we wouldn't need to do any extra work to paginate.

But regardless, the work here is done! Sorry for the delay. I had a bit of a health episode last week that kept me out pretty much the whole week 😭

@luke-engle
Copy link
Contributor Author

@timrogers Who could I get to review this change, and when can we get a new release out?

@timrogers
Copy link
Contributor

@luke-engle This looks good to me. Would you be able to re-push to see if we can get Actions to run the workflows?

@nickfloyd
Copy link
Contributor

@timrogers & @luke-engle I've unblocked the workflows - it was waiting for workflow approval... I'll keep an eye on it to ensure all checks are running as expected.

@nickfloyd
Copy link
Contributor

Looks like there is a debug statement in the tests that is causing a problem.

If that gets removed, I think that the workflows should be able to run complete again.

@luke-engle
Copy link
Contributor Author

Looks like there is a debug statement in the tests that is causing a problem.

If that gets removed, I think that the workflows should be able to run complete again.

Thanks Nick! That debug statement has been removed 👍

@timrogers
Copy link
Contributor

@luke-engle Looks like we still have a linting issue - you should be able to clean it up automatically with bundle exec rubocop -A.

@nickfloyd nickfloyd added the bug label Oct 11, 2022
@nickfloyd nickfloyd merged commit 8517612 into octokit:main Oct 11, 2022
@nickfloyd nickfloyd added Type: Bug Something isn't working as documented and removed bug labels Oct 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants