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

Bump per_page parameter to 100 in check_runs_for_ref call. #41

Merged
merged 2 commits into from
Sep 4, 2021
Merged

Bump per_page parameter to 100 in check_runs_for_ref call. #41

merged 2 commits into from
Sep 4, 2021

Conversation

ditman
Copy link
Contributor

@ditman ditman commented Aug 13, 2021

This passes the per_page parameter to the check_runs_for_ref octokit request.

(Apologies for the poor quality of this PR, I think I have a somewhat working testing environment, but I don't know much about ruby or how to contribute properly. All I could do was run the tests and see them pass (?)):

➜  app git:(bump-per-page-to-100) ✗ bundle exec rspec
......[#<OpenStruct status="in_progress", conclusion=nil, name="invoking_check">, #<OpenStruct status="completed", conclusion="success", name="check_completed_success">, #<OpenStruct status="completed", conclusion="failure", name="check_completed_failure">, #<OpenStruct status="completed", conclusion="neutral", name="check_completed_neutral">, #<OpenStruct status="completed", conclusion="timed_out", name="check_completed_timed_out">, #<OpenStruct status="completed", conclusion="action_required", name="check_completed_action_required">, #<OpenStruct status="queued", conclusion=nil, name="action_queued">]
...........

Finished in 0.03195 seconds (files took 0.43465 seconds to load)
17 examples, 0 failures

(I know this is very basic ruby, but I'm not even sure how to add a test to assert that we're calling the octokit client with the expected per_page: 100 value!)

@progapandist
Copy link
Collaborator

cc @matiasalbarello / @JoelLefkowitz — can one of you please take a look and maybe craft a new release with all recent changes? master is now veeeery different from the latest release. you should both have full access, as I'm leaving Le Wagon and won't be able to maintain this repo for a while. Hope you can pick up the banner! ❤️

@matiasalbarello
Copy link
Contributor

Hi @ditman. Thanks for the PR.
I was taking a read to the documentation from Octokit and found this issue (which is not actually an issue but just a request to improve their documentation). Long story short: I don't think we need to add the per_page option but rather setting auto_paginate true on the client instance creation (entrypoint.rb file). It's a rather simple change, would you like to add it?

@ditman
Copy link
Contributor Author

ditman commented Aug 26, 2021

@matiasalbarello oh yeah, auto_paginate would be a much better solution! I'll take a look to adding that ASAP.

(Apologies for the late response, I took a few days off!)

@ditman
Copy link
Contributor Author

ditman commented Sep 2, 2021

@matiasalbarello would it be possible to temporarily set the per_page parameter to 100, until the Octokit client supports the auto_paginate option for the check_runs_for_ref API method properly? (See this PR).

Copy link
Contributor

@matiasalbarello matiasalbarello left a comment

Choose a reason for hiding this comment

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

@matiasalbarello would it be possible to temporarily set the per_page parameter to 100, until the Octokit client supports the auto_paginate option for the check_runs_for_ref API method properly? (See this PR).

Of course! I did some tests here with >40 workflows and worked. So I'd say it's good to go. Thanks!

@matiasalbarello
Copy link
Contributor

I just merged master and fixed a little conflict.

@matiasalbarello matiasalbarello merged commit 5e93735 into lewagon:master Sep 4, 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.

3 participants