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.

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

Autorest Regen Preview to 3.0.0-alpha.20230713.1 by Mingzhe Huang from refs/pull/3584/head #37584

Conversation

azure-sdk
Copy link
Collaborator

@azure-sdk
Copy link
Collaborator Author

azure-sdk commented Jul 13, 2023

API change check

APIView has identified API level changes in this PR and created following API reviews.

Azure.Health.Insights.ClinicalMatching
Azure.Health.Insights.CancerProfiling
Azure.Analytics.Purview.Sharing
Azure.Analytics.Purview.Share

- update api signature
- add repeatable request headers into ignored headers of tests
@archerzz
Copy link
Member

archerzz commented Jul 13, 2023

The above SDKs are all in beta release. And Azure.Analytics.Purview.Share is even deprecated. It's fine to change the API signature.

public virtual Azure.Operation<Azure.Health.Insights.CancerProfiling.OncoPhenotypeResult> InferCancerProfile(Azure.WaitUntil waitUntil, Azure.Health.Insights.CancerProfiling.OncoPhenotypeData oncoPhenotypeData, string repeatabilityRequestId = null, System.DateTimeOffset? repeatabilityFirstSent = default(System.DateTimeOffset?), System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual System.Threading.Tasks.Task<Azure.Operation<System.BinaryData>> InferCancerProfileAsync(Azure.WaitUntil waitUntil, Azure.Core.RequestContent content, string repeatabilityRequestId = null, System.DateTimeOffset? repeatabilityFirstSent = default(System.DateTimeOffset?), Azure.RequestContext context = null) { throw null; }
public virtual System.Threading.Tasks.Task<Azure.Operation<Azure.Health.Insights.CancerProfiling.OncoPhenotypeResult>> InferCancerProfileAsync(Azure.WaitUntil waitUntil, Azure.Health.Insights.CancerProfiling.OncoPhenotypeData oncoPhenotypeData, string repeatabilityRequestId = null, System.DateTimeOffset? repeatabilityFirstSent = default(System.DateTimeOffset?), System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) { throw null; }
public virtual Azure.Operation<System.BinaryData> InferCancerProfile(Azure.WaitUntil waitUntil, Azure.Core.RequestContent content, Azure.RequestContext context = null) { throw null; }
Copy link
Member

Choose a reason for hiding this comment

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

I think this is the right fix but this would cause breaking change to health insight. @m-nash, do you have any concern for this change?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is the right fix but this would cause breaking change to health insight. @m-nash, do you have any concern for this change?

They are all beta versions. I think it's OK to change it.

Copy link
Member

Choose a reason for hiding this comment

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

yeah since they are in beta this is fine and expected

Copy link
Member

Choose a reason for hiding this comment

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

Hi,
It is ok by us that the repeatability headers will be set internally.
Related to the above change, I see that the Sync method was generated but the Async method dropped.
Any reason why?

Also, a question: is there an option to set the repeatability headers as context policy? (not see it used, it is just my curiosity)

Copy link
Member

Choose a reason for hiding this comment

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

Hi @asaflevi-ms are you referring to ClinicalMatchingClient? Which method do you refer to?
There are 4 methods being changed. 2 of them are sync methods, and other 2 are async methods.

As for the policy, the answer is yes. Users are free to set a pipeline policy through RequestContext. See here for example: https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/core/Azure.Core/samples/RequestContext.md#add-policy-for-request

Copy link
Member

Choose a reason for hiding this comment

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

Hi @archerzz
My mistake, I missed it.
I reviewed HealthInsights clients (ClinicalMatchingClient and CancerProfilingClient).
Approved by me.

archerzz and others added 3 commits July 17, 2023 14:57
- reverts commit 015e75d
- add backward compatible methods of `ChatRestClient` which still accepts
incoming `repeatability-request-id`
- revert change on test case base
- update test recordings to add repeatability request headers
- change test case base to use `IgnoredHeaders` instead of `LegacyExcludedHeaders`
archerzz and others added 2 commits July 17, 2023 23:45
- update test recordings to add repeatability request headers
- change test case base to use `IgnoredHeaders` instead of `LegacyExcludedHeaders`
@archerzz
Copy link
Member

Close since the changes have been merged in #37746

@archerzz archerzz closed this Jul 20, 2023
@azure-sdk azure-sdk deleted the auto-update-autorest-3.0.0-alpha.20230713.1 branch October 19, 2023 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants