Update LRO generation#1586
Merged
MaryGao merged 12 commits intoAzure:mainfrom Jan 31, 2023
Merged
Conversation
deyaaeldeen
reviewed
Sep 29, 2022
Member
deyaaeldeen
left a comment
deyaaeldeen
left a comment
There was a problem hiding this comment.
Thanks a lot for porting the changes! 🚀
I see there are two main failures in CI:
- clients that still need to be regenerated, would you mind regenerating them?
- test failures and these should be fixed once #1518 is merged in.
Merged
Closed
2 tasks
deyaaeldeen
added a commit
to Azure/azure-sdk-for-js
that referenced
this pull request
Jan 5, 2023
…te (#24159) ### Packages impacted by this PR @azure/core-lro ### Issues associated with this PR [Failures](https://dev.azure.com/azure-sdk/public/_build/results?buildId=2034536&view=logs&j=71cc2e7d-6454-5bf0-71b5-f1eef07a6c23&t=f2857310-5d68-5640-1409-0ba88dcfe076&l=1299) in Azure/autorest.typescript#1586 ### Describe the problem that is addressed by this PR `poll()` always sends the polling request as long as the operation has a location to poll from. However, once the operation is in a terminal state, there is no need to send the polling request. ### What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen? Sending the useless polling request is not the end of the world and won't affect the state of the poller. I don't feel strongly about the change but it is nice to not do unjustified extra work. ### Are there test cases added in this PR? _(If not, why?)_ Yes. ### Provide a list of related PRs _(if any)_ N/A ### Command used to generate this PR:**_(Applicable only to SDK release request PRs)_ ### Checklists - [x] Added impacted package name to the issue description - [ ] Does this PR needs any fixes in the SDK Generator?** _(If so, create an Issue in the [Autorest/typescript](https://github.com/Azure/autorest.typescript) repository and link it here)_ - [x] Added a changelog (if necessary)
deyaaeldeen
reviewed
Jan 16, 2023
deyaaeldeen
approved these changes
Jan 24, 2023
Member
deyaaeldeen
left a comment
deyaaeldeen
left a comment
There was a problem hiding this comment.
Thanks @joheredi for porting the changes!
We still need to update RLC LROs to not throw by enabling the core-lro flag but I can do that in another PR.
qiaozha
reviewed
Jan 28, 2023
qiaozha
reviewed
Jan 29, 2023
MaryGao
reviewed
Jan 30, 2023
qiaozha
approved these changes
Jan 31, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Porting changes from ##1568 after merging cadl-integration into main.
Updating LRO generation to the new API and adding a flag to generate using the legacy API
fixes #1598
Has dependency with issue: Azure/azure-sdk-for-js#24159