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

Remove upper constraint for Ms.Extensions. dependencies #2179

Merged
merged 7 commits into from
Jul 27, 2021

Conversation

cijothomas
Copy link
Member

@cijothomas cijothomas commented Jul 23, 2021

Once we get an ack from .NET runtime team that, Msft.Extensions.* packages will always remain backward compatible, we can remove the upper bound restriction on Msft.Extensions package. As of today, we have <6 allowed, blocking .NET 6 preview users.

This also ups the minimum version to be 3.1 (instead of 2.1), as this update will be released along 1.2.0 (Nov 2021), and 2.1* packages are out of support then.
The minimum version is kept as 2.1, to support Asp.Net Core 2.1 applications which target the .NET Framework.

Changes

Please provide a brief description of the changes here.

For significant contributions please make sure you have completed the following items:

  • CHANGELOG.md updated for non-trivial changes
  • Design discussion issue #
  • Changes in public API reviewed

@cijothomas cijothomas requested a review from a team July 23, 2021 01:12
@codecov
Copy link

codecov bot commented Jul 23, 2021

Codecov Report

Merging #2179 (1527820) into main (334689e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #2179   +/-   ##
=======================================
  Coverage   74.66%   74.66%           
=======================================
  Files         217      217           
  Lines        6974     6974           
=======================================
  Hits         5207     5207           
  Misses       1767     1767           

@cijothomas
Copy link
Member Author

@reyang
Copy link
Member

reyang commented Jul 23, 2021

We are still working with .NET runtime team to clarify the support policy - Microsoft.Extension.Logging version 2.1.x might be used in some old version of ASP.NET (not ASP.NET Core) which has their own lifecycle.
Either we hold this PR for now until we have good clarification, or we merge it for now and prepare for a potential revert/rollback.

@cijothomas
Copy link
Member Author

some old version of ASP.NET (not ASP.NET Core) which has their own lifecycle.

I think this is Asp.Net Core (on .NET Framework), and not the "pure" Asp.Net. For such projects, the targetframework in the csproj would be net46 or net47 etc, but the nuget packages are M.E.** 2.1.*...

@cijothomas cijothomas requested a review from reyang July 27, 2021 04:06
@cijothomas
Copy link
Member Author

@shirhatti Please check if this looks good!

@cijothomas
Copy link
Member Author

We are still working with .NET runtime team to clarify the support policy - Microsoft.Extension.Logging version 2.1.x might be used in some old version of ASP.NET (not ASP.NET Core) which has their own lifecycle.
Either we hold this PR for now until we have good clarification, or we merge it for now and prepare for a potential revert/rollback.

Updated the changes and description accounting for this scenario. Please re-review.

Copy link

@shirhatti shirhatti left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Copy link
Member

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM.

@reyang reyang added pr:please-merge pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package logs Logging signal related and removed pr:do-not-merge labels Jul 27, 2021
@cijothomas cijothomas merged commit 4df5121 into main Jul 27, 2021
@cijothomas cijothomas deleted the cijothomas/iloggerrestrictionremoval branch July 27, 2021 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
logs Logging signal related pkg:OpenTelemetry Issues related to OpenTelemetry NuGet package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants