Skip to content

feat!: Add support for pagination options in rules API methods #3562

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ktruedat
Copy link

@ktruedat ktruedat commented Apr 24, 2025

BREAKING CHANGE: GetRulesForBranch, GetAllRulesets, and GetAllRepositoryRulesets now accept opts.

Updated GetRulesForBranch, GetAllRulesets, and GetAllRepositoryRulesets methods to accept optional pagination parameters (ListOptions). Enhanced test cases to validate the use of these parameters in API requests.

Fixes: #3560

Updated `GetRulesForBranch`, `GetAllRulesets`, and `GetAllRepositoryRulesets` methods to accept optional pagination parameters (`ListOptions`). Enhanced test cases to validate the use of these parameters in API requests.
@gmlewis gmlewis changed the title Add support for pagination options in rules API methods feat!: Add support for pagination options in rules API methods Apr 24, 2025
@gmlewis gmlewis added NeedsReview PR is awaiting a review before merging. Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels Apr 24, 2025
Copy link

codecov bot commented Apr 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.24%. Comparing base (6a7684f) to head (3d1050a).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3562      +/-   ##
==========================================
+ Coverage   91.23%   91.24%   +0.01%     
==========================================
  Files         183      183              
  Lines       16053    16075      +22     
==========================================
+ Hits        14646    14668      +22     
  Misses       1231     1231              
  Partials      176      176              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Please make sure to run the scripts mentioned in step 4 of CONTRIBUTING.md and push the changes (not force-push) to this PR. Thank you!

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Also, to fix the CodeCov issues, please add testBadOptions to your new tests. You can see examples in other locations.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @ktruedat!
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@stevehipwell - might you have time for a code review? Thank you!

Copy link
Contributor

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

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

Do we not want tests to cover the case where we set opts to nil? I'd have assumed that the existing tests would use nil and new tests would have been added for the pagination pattern?

@gmlewis
Copy link
Collaborator

gmlewis commented Apr 25, 2025

Testing with opts being nil sounds like a good idea in this case.
In the general case, that would be uninteresting, but it would probably be good to show it, although we still can't see how GitHub interprets the missing query parameter... so I'm fine either way.

@stevehipwell
Copy link
Contributor

@gmlewis wouldn't testing without opts catch a regression with the method where it no longer supported nil?

@gmlewis
Copy link
Collaborator

gmlewis commented Apr 25, 2025

@gmlewis wouldn't testing without opts catch a regression with the method where it no longer supported nil?

Yes, great idea. I like it. Thank you, @stevehipwell!
@ktruedat - could you please add that to this PR?

@ktruedat
Copy link
Author

@gmlewis of course! I actually wanted to do separate tests from the start but wasn't sure about it. Will add them in a few

@ktruedat
Copy link
Author

@gmlewis @stevehipwell I included separate tests with ListOptions for every modified operation. I didn't include the full mock response in the client response since we are not really interested in the response itself, but rather testing the existence of the query params. Let me know if that's okay, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). NeedsReview PR is awaiting a review before merging.
Projects
None yet
3 participants