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

Removes Experimental attribute from ResilienceHandler class #5670

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

iliar-turdushev
Copy link
Contributor

@iliar-turdushev iliar-turdushev commented Nov 19, 2024

The PR removes Experimental attribute from ResilienceHandler class.

Fixes #5669

Microsoft Reviewers: Open in CodeFlow

Removes experimental attribute from ResilienceHandler class
Adds new experimental and stable APIs to the API json file
@iliar-turdushev iliar-turdushev requested a review from a team as a code owner November 19, 2024 13:48
@amadeuszl amadeuszl self-requested a review November 19, 2024 13:52
@@ -16,7 +16,7 @@ Write-Output "Installing required toolset"
InitializeDotNetCli -install $true | Out-Null

$Project = $PSScriptRoot + "/../eng/Tools/ApiChief/ApiChief.csproj"
$Command = $PSScriptRoot + "/../artifacts/bin/ApiChief/Debug/net8.0/ApiChief.dll"
$Command = $PSScriptRoot + "/../artifacts/bin/ApiChief/Debug/net9.0/ApiChief.dll"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We now build the ApiChief tool for net9.0 that is why I changed this.

@@ -1,5 +1,5 @@
{
"Name": "Microsoft.Extensions.Http.Resilience, Version=8.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35",
"Name": "Microsoft.Extensions.Http.Resilience, Version=9.1.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The API json file hasn't been updated since dotnet/extensions 8.0.0 therefore the changes to this file also include APIs that were changed/updated earlier.

Suppress LA0006 warning causing the build pipeline to fail
<!-- Disable "Published symbols cannot be deleted to maintain compatibility" warning
because we have different APIs for different TFMs. -->
<NoWarn Condition="'$(TargetFramework)' == 'net462'">$(NoWarn);LA0006</NoWarn>
</PropertyGroup>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to suppress LA0006 - Published symbols cannot be deleted to maintain compatibility warning. The reason is the following. Together with making the ResilienceHandler class stable, we also made stable protected members SendAsync and Send. These members are inherited from DelegatingHandler and ResilienceHandler overrides them. The Send method is not available on net462 and this causes the build pipeline to fail with the LA0006 warning. To make the build pipeline "green" we have to suppress the LA0006 warning.

Copy link
Member

Choose a reason for hiding this comment

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

This is suppressing all of the warnings on public symbols though, right? Is there not a way to suppress that single change?

Copy link
Member

Choose a reason for hiding this comment

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

Also, I'm not very familiar to which analyzer is throwing those errors, but FWIW we do run package validation when we pack projects, which would catch compatibility issues in packages. In this case, net462 and net6.0+ aren't even supposed to be compatible with each other so perhaps the analyzer tooling being used here is not correct.

@RussKie
Copy link
Member

RussKie commented Nov 22, 2024

Approving on the assumption the @dotnet/dotnet-extensions-resilience team has done the due diligence for the API review.

@RussKie RussKie added area-resilience api-approved API was approved in API review, it can be implemented labels Nov 22, 2024
@iliar-turdushev
Copy link
Contributor Author

iliar-turdushev commented Nov 22, 2024

Tagging @RussKie and @joperezr as infra-area owners. The PR touches some infra files. Please, review it. Thank you.

Approving on the assumption the @dotnet/dotnet-extensions-resilience team has done the due diligence for the API review.

Making the API stable was discussed there #4893 (comment). The API looks solid, I think we are good to make it stable.

Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

Left a couple of minor comments, but looks good otherwise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved in API review, it can be implemented area-resilience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Http.Resilience] Make experimental class ResilienceHandler stable
4 participants