Skip to content

Add pagination to Costmanagement Query API#29381

Closed
GuillaumeColom wants to merge 2 commits intoAzure:mainfrom
GuillaumeColom:add-pagination-to-cost-management-query-api
Closed

Add pagination to Costmanagement Query API#29381
GuillaumeColom wants to merge 2 commits intoAzure:mainfrom
GuillaumeColom:add-pagination-to-cost-management-query-api

Conversation

@GuillaumeColom
Copy link

Description

Add Pagination capability to Costmanagement - Query API

All SDK Contribution checklist:

  • The pull request does not introduce [breaking changes]
    Introduce some breaking changes by adding skipToken parameter to methods in Query API

  • CHANGELOG is updated for new features, bug fixes or other significant changes.

  • I have read the contribution guidelines.

General Guidelines and Best Practices

  • Title of the pull request is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

@ghost ghost added the customer-reported Issues that are reported by GitHub users external to the Azure organization. label Jun 10, 2022
@ghost
Copy link

ghost commented Jun 10, 2022

Thank you for your contribution GuillaumeColom! We will review the pull request and get back to you soon.

@ghost
Copy link

ghost commented Jun 10, 2022

CLA assistant check
All CLA requirements met.

@HostParam("$host") String endpoint,
@PathParam(value = "scope", encoded = true) String scope,
@QueryParam("api-version") String apiVersion,
@QueryParam("$skiptoken") String skiptoken,

Choose a reason for hiding this comment

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

@ghost
Copy link

ghost commented Aug 12, 2022

Hi @GuillaumeColom. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@GuillaumeColom
Copy link
Author

Hello,

We are waiting another pull request to be completed (for API spec) before pursuing here.

Thank you.

@ghost ghost removed the no-recent-activity There has been no recent activity on this issue. label Aug 18, 2022
@neiser
Copy link

neiser commented Sep 7, 2022

@GuillaumeColom Thank you very much for this contribution! I highly appreciate it. The blocking PR Azure/azure-rest-api-specs#19609 appears to have been merged now. I hope this one can then be merged as well soon. Eagerly waiting for it!

@neiser
Copy link

neiser commented Sep 10, 2022

@weidongxu-microsoft Any chance of getting this merged soon? Do you have a timeline? Sorry for bugging you with this, but I'm actually blocked by this issue and I'm considering copying this patch manually into my project unless this gets merged/released within the next few days. So, if you already know it will take some weeks until it's merged, please let me know. Thank you!

@neiser
Copy link

neiser commented Sep 12, 2022

@GuillaumeColom After looking into this code change in more detail, I think exposing skipToken in com.azure.resourcemanager.costmanagement.models.Queries#usage is a bad idea, as it's introducing a breaking change. Pagination should be handled similar how the com.azure.resourcemanager.costmanagement.implementation.OperationsClientImpl#listAsync() is doing it by chaining the requests.

@weidongxu-microsoft
Copy link
Member

@neiser

Romain is checking with cost management service to sign off on swagger Azure/azure-rest-api-specs#19609

After it signed-off, Java will regenerate the whole SDK with 2022-08-01-preview api-version.


For the pageable, it is also due to swagger. If service supports it correctly, it should be annotated with x-ms-pageable, e.g. https://github.com/Azure/azure-rest-api-specs/blob/main/specification/cost-management/resource-manager/Microsoft.CostManagement/preview/2022-08-01-preview/costmanagement.json#L70-L72

@ghost ghost added the no-recent-activity There has been no recent activity on this issue. label Nov 18, 2022
@ghost
Copy link

ghost commented Nov 18, 2022

Hi @GuillaumeColom. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

@ghost ghost closed this Nov 25, 2022
@ghost
Copy link

ghost commented Nov 25, 2022

Hi @GuillaumeColom. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing "/reopen" if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the "no-recent-activity" label; otherwise, this is likely to be closed again with the next cleanup pass.

@Totox64
Copy link

Totox64 commented Jul 4, 2023

Hello @weidongxu-microsoft, I would like to know if it's possible to reopen this PR. Since the PR in the rest api spec is merged, this sould be quite straight forward

@Totox64
Copy link

Totox64 commented Jul 4, 2023

/reopen

@github-actions
Copy link
Contributor

github-actions bot commented Jul 4, 2023

Sorry, @Totox64, only the original author can reopen this pull request.

@weidongxu-microsoft
Copy link
Member

weidongxu-microsoft commented Jul 5, 2023

@Totox64
backend decided not to change (issue here Azure/azure-rest-api-specs#23405)

java provided a workaround here #34455

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

customer-reported Issues that are reported by GitHub users external to the Azure organization. no-recent-activity There has been no recent activity on this issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants