Skip to content

Proper Success Response Handling #272

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

Merged
merged 11 commits into from
Jul 15, 2020
Merged

Proper Success Response Handling #272

merged 11 commits into from
Jul 15, 2020

Conversation

peombwa
Copy link
Member

@peombwa peombwa commented Jun 23, 2020

This PR closes #110 and #198 by adding OverrideOnDefault method on PSCmdlet. It will ensure cmdlets won't fail when a service returns a success code that's not defined in the OpenAPI coc. The OverrideOnDefault will be added to the generated cmdlet at generation time by the directive in readme.graph.md.

The generated code will call OverrideOnDefault after considering the status code defined in the OpenAPI doc before giving a response a blanket success if its status code is 2xx as shown below.
image
image

The PR also closes #271 by allowing customers to specify a custom -PageSize when iterating through collection of pages as described in #275.
image

Checklist Before Merge

AB#5066
AB#5067

@peombwa peombwa self-assigned this Jun 23, 2020
@KirkMunro
Copy link

One thing to consider with this as a fix for #271:

Making -PageSize properly control the page size while allowing users to retrieve all records is the right thing to do.

But -PageSize currently has -Limit and -Top as aliases, and neither of those aliases are appropriate for -PageSize because they are about limiting the overall number of records returned, not about controlling the size of the pages used to retrieve those records.

I think -Limit should be broken out into its own parameter with -Top as an alias to work like -PageSize did before this change.

Related: #275

ddyett
ddyett previously approved these changes Jun 29, 2020
ddyett
ddyett previously approved these changes Jul 10, 2020
@peombwa
Copy link
Member Author

peombwa commented Jul 10, 2020

Due to COGs concerns, we will not fetch all records from the service by default unless the user explicitly opts-in via -All parameter. See discussion here.

@peombwa
Copy link
Member Author

peombwa commented Jul 13, 2020

@ddyett, @darrelmiller, I've updated the PR to address the COGs concerns with fetching all pages by default. With the update made in d4f8797, we will fetch a single page of 100 items by default. -All parameter can then be used to fetch all items, and -PageSize to specify your desired page size. The new behavior will be as follows:

image
cc @KirkMunro

@peombwa peombwa requested a review from ddyett July 13, 2020 20:07
@peombwa peombwa merged commit 1304cdd into dev Jul 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow customers to iterate collection of pages based on their PageSize/top. Update-MgUserEvent throws expeption but is running fine
3 participants