-
Notifications
You must be signed in to change notification settings - Fork 249
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
add api-logs-limit flag #777
Conversation
Should we return an API error if the user specify more than the limit? Because if the user requests 5000 and gets 1000, then it looks like there are no more results, and they will stop making paginated requests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Structure is solid, and the limit makes sense to happen at the api lvl.
Probably worth to have a unit test that ensures the filtering is returning the max number of results in the events_test.go
and in the transfers_test.go
. We're testing the NormalizeOptions
but if for example that function gets left out for some reason the tests won't capture that the query becomes now unbounded ?
Also, perhaps update the api yml definition to account for the existence of the flag ?
677850f
to
a9c8682
Compare
@libotony I made a PR here to fix the E2E. We need to merge this to main and then put the commit SHA in the e2e GHA |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #777 +/- ##
==========================================
- Coverage 62.62% 62.51% -0.11%
==========================================
Files 199 199
Lines 18118 18140 +22
==========================================
- Hits 11346 11340 -6
- Misses 5690 5718 +28
Partials 1082 1082 ☔ View full report in Codecov by Sentry. |
2e2d893
to
3ffd6ee
Compare
Description
Add a flag to allow node runner to specify the logs limit for the logs filter API.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also
list any relevant details for your test configuration
Test Configuration:
Checklist: