-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(logging/logadmin): allow logging PageSize to override #9409
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
Could you add a test for this in TestListLogEntriesRequest
in logadmin_test.go
?
Hey @gkevinzheng , I've pushed the requested changes. Can you please review it again? |
@ShubhamRasal Could you add a test where no option for |
- remove duplicated test case
To address your concern, I want to clarify that the scenario you mentioned is indeed covered by our existing test suite. Initially, our test cases were designed without explicitly naming the variables, leading us to use However, recognizing the importance of readability and ease of contribution, I have recently revisited and updated our test cases. These updates aim to make the test intentions clearer and the overall tests more contributor-friendly. During this process, I also identified and removed a duplicate test case. I hope these adjustments effectively address your concern. Please let me know if there are any further adjustments or clarifications you would recommend. |
resolve #5031
This PR, allows user to support custom page size.