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

feat(logging/logadmin): allow logging PageSize to override #9409

Merged
merged 10 commits into from Mar 6, 2024

Conversation

ShubhamRasal
Copy link
Contributor

@ShubhamRasal ShubhamRasal commented Feb 12, 2024

resolve #5031

This PR, allows user to support custom page size.

Copy link

google-cla bot commented Feb 12, 2024

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.

@product-auto-label product-auto-label bot added the size: xs Pull request size is extra small. label Feb 12, 2024
@codyoss codyoss changed the title feat: allow logging PageSize to override feat(logging/logadmin): allow logging PageSize to override Feb 12, 2024
@product-auto-label product-auto-label bot added the api: logging Issues related to the Cloud Logging API. label Feb 13, 2024
Copy link
Contributor

@gkevinzheng gkevinzheng left a 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?

@product-auto-label product-auto-label bot added size: s Pull request size is small. and removed size: xs Pull request size is extra small. labels Feb 16, 2024
@ShubhamRasal
Copy link
Contributor Author

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?

@gkevinzheng
Copy link
Contributor

@ShubhamRasal Could you add a test where no option for PageSize is set in the EntriesOption? I'd like to make sure the change doesn't break existing functionality? It shouldn't, but just in case.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Feb 29, 2024
@ShubhamRasal
Copy link
Contributor Author

ShubhamRasal commented Feb 29, 2024

@ShubhamRasal Could you add a test where no option for PageSize is set in the EntriesOption? I'd like to make sure the change doesn't break existing functionality? It shouldn't, but just in 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 0 as a default value for PageSize in all tests to simulate the absence of this option.

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.

image (6)

I hope these adjustments effectively address your concern. Please let me know if there are any further adjustments or clarifications you would recommend.
@gkevinzheng

@gkevinzheng gkevinzheng added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2024
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2024
@gkevinzheng gkevinzheng merged commit 5ca0271 into googleapis:main Mar 6, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the Cloud Logging API. size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

logging/logadmin: specify pagesize for entries listing request
3 participants