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

fix(get): propagate next page size param #9029 #9503

Merged
merged 22 commits into from
Nov 1, 2022

Conversation

jacksjm
Copy link
Contributor

@jacksjm jacksjm commented Oct 7, 2022

Summary

Fixed to propagate size parameter to perform next request

Full changelog

  • Fixed to propagate the size parameter to perform the next request, ensuring the same size in requests

Issue reference

Fix #9029

@jacksjm jacksjm requested a review from a team as a code owner October 7, 2022 13:51
@CLAassistant
Copy link

CLAassistant commented Oct 7, 2022

CLA assistant check
All committers have signed the CLA.

kong/api/endpoints.lua Outdated Show resolved Hide resolved
@chronolaw
Copy link
Contributor

Great Job, Thank you Jackson.

Because I am not familiar with admin api, I think that we need others to take a look then approve.

@jacksjm
Copy link
Contributor Author

jacksjm commented Oct 10, 2022

Great Job, Thank you Jackson.

Because I am not familiar with admin api, I think that we need others to take a look then approve.

Thanks @chronolaw. This is OK. could you tag a sponsor to review? I do not know them. Do you need to remove your change request?

kong/api/endpoints.lua Outdated Show resolved Hide resolved
@jacksjm
Copy link
Contributor Author

jacksjm commented Oct 11, 2022

@mayocream would you be on the admin api team for review?

Copy link
Contributor

@mayocream mayocream left a comment

Choose a reason for hiding this comment

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

Hi, although it’s a small change, but a proper test case should be written to ensure the functionality works.

@jacksjm
Copy link
Contributor Author

jacksjm commented Oct 21, 2022

Hi, although it’s a small change, but a proper test case should be written to ensure the functionality works.

Thanks for your feedback @mayocream!!! I searched the admin_api tests for next_page validators but couldn't find them. I believe there is no validation on this return, if I am wrong please correct me. Do you have any validation suggestions for this scenario?

Copy link
Contributor

@mayocream mayocream left a comment

Choose a reason for hiding this comment

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

Should be in this format:

< local next_page = offset and fmt("/%s?offset=%s%s",
---
> local next_page = offset and fmt("/%s?offset=%s%s%s",

@mayocream
Copy link
Contributor

I think this would be a feature (although very small); I'll include @sumimakito in this topic to see if it is good for Kong Manager (UI).

@sumimakito
Copy link
Member

@jacksjm Thank you for your work 🙌
@mayocream Yes - I believe this would benefit KM and other projects that rely on the next link in the response, as long as we can ensure this change would not break them.

@jacksjm jacksjm requested review from hbagdi and mayocream and removed request for hbagdi October 28, 2022 14:58
@jacksjm jacksjm requested review from hbagdi and mayocream and removed request for mayocream and hbagdi October 29, 2022 00:57
@fffonion fffonion dismissed mayocream’s stale review November 1, 2022 04:14

dismiss as addressed

@fffonion
Copy link
Contributor

fffonion commented Nov 1, 2022

@jacksjm Thanks for the contribution, this is now merged 🎉 !

fffonion added a commit that referenced this pull request Nov 1, 2022
@jacksjm
Copy link
Contributor Author

jacksjm commented Nov 1, 2022

@jacksjm Thanks for the contribution, this is now merged 🎉 !

I'm very glad to contribute!!!

fffonion added a commit that referenced this pull request Nov 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Admin API query parameter size is not reported in the next URL in paginated lists
7 participants