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.

Sign up for GitHub

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

Jump to bottom

Add scope of waitForCompletion in LRO #29033

Merged
merged 15 commits into from
Jun 22, 2022
Merged

Add scope of waitForCompletion in LRO #29033

merged 15 commits into from
Jun 22, 2022

Conversation

pshao25
Copy link
Member

@pshao25 pshao25 commented May 30, 2022

@ghost ghost added the Azure.Core label May 30, 2022
@annelo-msft annelo-msft requested a review from AlexanderSher June 1, 2022 00:12
@pshao25 pshao25 requested review from archerzz and chunyu3 June 1, 2022 07:07
@chunyu3
Copy link
Member

chunyu3 commented Jun 1, 2022

Hi @pshao25 So now Will LRO set two scopes, one is 'WaitForcomplete', the other is 'UpdateStatus'?

@pshao25
Copy link
Member Author

pshao25 commented Jun 2, 2022

Hi @pshao25 So now Will LRO set two scopes, one is 'WaitForcomplete', the other is 'UpdateStatus'?

Yes

@pshao25
Copy link
Member Author

pshao25 commented Jun 2, 2022 •
edited
Loading

@AlexanderSher @chunyu3 Considering your suggesting about changing flag to string. I would add a check whether the input name is _updateStatusScopeName or _waitForCompletionScopeName because I really don't want to allow passing in arbitrary name. In that case, we might not put the _updateStatusScopeName into OperationInternal<T> and I have to make it from private to protected.

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

annelo-msft
annelo-msft reviewed Jun 3, 2022
View reviewed changes
Copy link
Member

@annelo-msft annelo-msft left a comment

Choose a reason for hiding this comment

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

What tests validate that this functionality is correct?

@pshao25
Copy link
Member Author

pshao25 commented Jun 6, 2022 •
edited
Loading

What tests validate that this functionality is correct?

We will comment these lines to test this scenario https://github.com/Azure/autorest.csharp/blob/feature%2Fv3/test/AutoRest.TestServerLowLevel.Tests/dpg-customization.cs#L106-L108
Meanwhile, we will remove the scopes added for grow-up story.

@annelo-msft
Copy link
Member

We will comment these lines to test this scenario https://github.com/Azure/autorest.csharp/blob/feature%2Fv3/test/AutoRest.TestServerLowLevel.Tests/dpg-customization.cs#L106-L108

These are changes to Azure.Core files in azure-sdk-for-net. We should have test coverage showing their correctness in azure-sdk-for-net.

@pshao25
Copy link
Member Author

pshao25 commented Jun 7, 2022

We will comment these lines to test this scenario https://github.com/Azure/autorest.csharp/blob/feature%2Fv3/test/AutoRest.TestServerLowLevel.Tests/dpg-customization.cs#L106-L108

These are changes to Azure.Core files in azure-sdk-for-net. We should have test coverage showing their correctness in azure-sdk-for-net.

Added.

@pshao25 pshao25 mentioned this pull request Jun 7, 2022
Merged
annelo-msft
annelo-msft reviewed Jun 7, 2022
View reviewed changes
Copy link
Member

@annelo-msft annelo-msft left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We should have end-to-end tests for distributed tracing in long-running operations, validating each of the cases described here: Azure/autorest.csharp#2136 (comment)

Update: @AlexanderSher let me know the end-to-end tests are here: https://github.com/Azure/autorest.csharp/blob/feature/v3/test/AutoRest.TestServerLowLevel.Tests/dpg-customization.cs#L96.

We should move these into Azure.Core when we migrate these types over.

@pshao25 pshao25 mentioned this pull request Jun 8, 2022
Closed
@pshao25
Copy link
Member Author

pshao25 commented Jun 17, 2022

/azp run net - core - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

annelo-msft
annelo-msft approved these changes Jun 17, 2022
View reviewed changes
Copy link
Member

@annelo-msft annelo-msft left a comment

Choose a reason for hiding this comment

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

All of the tests I've requested are present and pass. Please address @AlexanderSher's comments before merging.

annelo-msft
annelo-msft approved these changes Jun 22, 2022
View reviewed changes
Copy link
Member

@annelo-msft annelo-msft left a comment

Choose a reason for hiding this comment

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

You can merge the PR with these changes once the CI is passing. Thanks, @pshao25!

@pshao25 pshao25 merged commit 465aa61 into Azure:main Jun 22, 2022
zhihaoxue pushed a commit to zhihaoxue/azure-sdk-for-net that referenced this pull request Jul 27, 2022
@pshao25 @zhihaoxue
Add scope of waitForCompletion in LRO (Azure#29033)
57d9716 
* Add scope of waitForCompletion in LRO

* Change flag to string

* Resolve comments

* Resolve comments

* Add tests

* Resolve comments

* Fix tests

* Fix test

* Fix test

* Resolve comments

* Resolve comments

* Make scope name not nullable.

* Resolve comments

* Remove useless method

* Resolve comment

Co-authored-by: Pan Shao <pashao@microsoft.com>
sofiar-msft pushed a commit to sofiar-msft/azure-sdk-for-net that referenced this pull request Dec 7, 2022
@pshao25 @sofiar-msft
Add scope of waitForCompletion in LRO (Azure#29033)
5d11b61 
* Add scope of waitForCompletion in LRO

* Change flag to string

* Resolve comments

* Resolve comments

* Add tests

* Resolve comments

* Fix tests

* Fix test

* Fix test

* Resolve comments

* Resolve comments

* Make scope name not nullable.

* Resolve comments

* Remove useless method

* Resolve comment

Co-authored-by: Pan Shao <pashao@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexanderSher AlexanderSher AlexanderSher left review comments

@lirenhe lirenhe lirenhe left review comments

@chunyu3 chunyu3 chunyu3 left review comments

@annelo-msft annelo-msft annelo-msft approved these changes

@JoshLove-msft JoshLove-msft Awaiting requested review from JoshLove-msft

@christothes christothes Awaiting requested review from christothes

@KrzysztofCwalina KrzysztofCwalina Awaiting requested review from KrzysztofCwalina

@archerzz archerzz Awaiting requested review from archerzz

Assignees
No one assigned
Labels
Azure.Core DPG
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

WaitForCompletion scope is not included in the LowLevelFuncOperation
7 participants
@pshao25 @chunyu3 @azure-sdk @annelo-msft @AlexanderSher @lirenhe @JoshLove-msft