-
Notifications
You must be signed in to change notification settings - Fork 287
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
AspNetCore & FrameworkReference #2412
Conversation
|
||
/// <summary> | ||
/// Initializes a new instance of the <see cref="AspNetCoreEnvironmentTelemetryInitializer"/> class. | ||
/// </summary> | ||
/// <param name="environment">HostingEnvironment to provide EnvironmentName to be added to telemetry properties.</param> | ||
public AspNetCoreEnvironmentTelemetryInitializer(IHostingEnvironment environment) | ||
public AspNetCoreEnvironmentTelemetryInitializer(IHostEnvironment environment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this'd be considered breaking change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure the best way to handle this. The former type appears to have been removed from .NET.
Error Message:
'IHostingEnvironment' is obsolete: 'This type is obsolete and will be removed in a future version.
The recommended alternative is Microsoft.AspNetCore.Hosting.IWebHostEnvironment.'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, i think i have a fix for this.
Marking the former constructor using IHostingEnvironment
as Obsolete fixes the build errors without breaking the PublicApi.
And then I added a new constructor using the newer IHostEnvironment
for NetCore3.1 and Net5.0 support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think i have a comprehensive fix for this.
Original constructor expected IHostingEnvironment
. <-- now obsolete
I've added two new constructors;
- one expecting
IHostEnvironment
. <-- This is now the recommended approach. - and one expecting both
IHostEnvironment
andIHostingEnvironment
. <-- for backcompat
It appears that the framework can add BOTH to the Dependency Injection was causing the SDK initialization to fail because of "ambiguous constructors".
The constructor with both params solves this issue.
Fix Issue #2251.
Convert AspNetCore and AspNetCore.Tests projects to use FrameworkReference.
Todo
Changes
IHostingEnvironment
has been removed from the framework. In our SDK, replaced withIHostEnvironment
.see also: https://weblog.west-wind.com/posts/2020/Feb/26/Working-with-IWebHostEnvironment-and-IHostingEnvironment-in-dual-targeted-NET-Core-Projects
Checklist
For significant contributions please make sure you have completed the following items:
The PR will trigger build, unit tests, and functional tests automatically. Please follow these instructions to build and test locally.
Notes for authors:
Notes for reviewers:
/AzurePipelines run
will queue all builds/AzurePipelines run <pipeline-name>
will queue a specific build