Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

feat: pagination with templates #150

Merged
merged 22 commits into from
Sep 27, 2023
Merged

feat: pagination with templates #150

merged 22 commits into from
Sep 27, 2023

Conversation

t0mk
Copy link
Contributor

@t0mk t0mk commented Sep 12, 2023

This PR adds logic for generating listing functions capable of going through all the pages (think devices in a project, or events in a device).

It adds templates for the new code, docs, and code tests. The templates are fetched from oag v7.0.0, and then modified for the sake of the paginated listers.

It uses vendor extension property object in the form

     x-paginated:
        x-paginated-property: Events

The spec patching part can be done in other oag generated SDKs.

related to #135

fixes #131

Makefile Outdated Show resolved Hide resolved
config.yaml Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@t0mk t0mk changed the title Pagination with templates feat: pagination with templates Sep 13, 2023
@t0mk
Copy link
Contributor Author

t0mk commented Sep 14, 2023

@ctreatma

In the API specs, there are several operations that have the page etc. params, but their response schema doesn't have the meta object (where there's the page number of the resource list in response).

Namely:

  • op findVirtualCircuitEvents, Response schema: Event (singular)
  • op findVrfRouteEventsRequest, Response schema: Event (singular)
  • op findProjectLicenses, Response schema: LicenseList

This is something that could be checked automatically when creating new api doc. In naive code:

for p in paths:
  if "Page" in p.get.params:
    assert("meta" in p.get.response.schema)

@ctreatma
Copy link
Contributor

@sbhatnagar-equinix take a look at @t0mk's comment above; he has a good suggestion in there to proactively catch missing properties for paginated endpoints.

@t0mk
Copy link
Contributor Author

t0mk commented Sep 14, 2023

@ctreatma @displague Another iteration of the pagination code generation. This time I managed to actually do a paginated call in go, using the generated code from the new template.

I updated the pytohn script to add also the property name, you can see it
c5edb88

I updated the template so that it uses the property and so that the code actually works:
4ddb78f
(scroll down)

Example of generated code:
https://github.com/equinix-labs/metal-go/blob/pagination_with_templates/metal/v1/api_devices_paginated_listers.go

@t0mk
Copy link
Contributor Author

t0mk commented Sep 20, 2023

Make the prefix x-equnix-metal-pagination and x-equinix-metal-paginated-property, hopefully there no limit for the property name. @ctreatma @displague

docs/HardwareReservationsApi.md Outdated Show resolved Hide resolved
@t0mk
Copy link
Contributor Author

t0mk commented Sep 25, 2023

@ctreatma @displague I rebased and cleaned this PR. You can see the gradual changes in the commits added today (25-09-2023). It's big but quite structured.

I didn't have to do any change to the Makefile 🤔

This is ready for review.

@t0mk t0mk marked this pull request as ready for review September 25, 2023 12:41
displague
displague previously approved these changes Sep 25, 2023
Copy link
Member

@displague displague left a comment

Choose a reason for hiding this comment

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

lgtm

I left some nitpics about generated function comments and documentation. I'd prefer for those to land in this merge, but open to follow-ups.

Some thoughts on the vendoring:

  • I think we may want to use an x-equinix-metal- prefix.
  • I don't know that we need to nest the x-paginated and x-paginated-property, it could be just the latter, flat (I think)

Since these are only used internally, we can rename them in this project at any time (not merge blocking).

@t0mk
Copy link
Contributor Author

t0mk commented Sep 25, 2023

@displague I will include your suggestions in this PR. I will also change the property name to x-equinix-metal-paginated-property.

Co-authored-by: Marques Johansson <mjohansson@equinix.com>
Signed-off-by:  Tomáš Karásek <t0mk@users.noreply.github.com>
@t0mk
Copy link
Contributor Author

t0mk commented Sep 25, 2023

@displague all changes done

  • chanted property to x-equinix-metal-paginated-property
  • changed comments to your suggestions
  • regenerated everything

Please review again.

@t0mk t0mk requested a review from displague September 25, 2023 15:20
@t0mk t0mk merged commit 621919d into main Sep 27, 2023
3 checks passed
@t0mk t0mk deleted the pagination_with_templates branch September 27, 2023 19:53
@github-actions
Copy link
Contributor

This PR is included in version 0.23.0 🎉

@ctreatma ctreatma mentioned this pull request Dec 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a way to automatically load paginated resources
4 participants