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

#679 Implement NullScope for netcore 3.1 packages compatibility #683

Merged
merged 3 commits into from
Jun 25, 2020
Merged

#679 Implement NullScope for netcore 3.1 packages compatibility #683

merged 3 commits into from
Jun 25, 2020

Conversation

wdolek
Copy link
Contributor

@wdolek wdolek commented Jun 2, 2020

Issue:
#679

Description of changes:
Implementing NullScope which became internal in Microsoft.Extensions.Logging.Abstractions 3.1.

Note: I wasn't able to compile whole solution, couldn't find any developer instructions. Unit tests for logging are passing.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@SrutiG SrutiG self-requested a review June 5, 2020 19:15
<PackageReference Include="Microsoft.Extensions.Logging" Version="2.1.0" />
<PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="2.1.0" />
<PackageReference Include="Microsoft.Extensions.Configuration.Abstractions" Version="2.1.0" />
<PackageReference Include="Microsoft.Extensions.Logging" Version="[3.1.0,3.2.0)" />
Copy link
Member

Choose a reason for hiding this comment

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

This library still needs to work with ASP.NET Core 2.1 Lambda functions so i don't want to bump the version to 3.1 as the minimum. Is there a reason we need to do that? Also what is the goal of the using a range blocking at 3.2? That would prevent usage for people that want to use this with .NET 5 preview using custom runtimes.

I think we can just take this part out of the PR and then release the NullScope fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@normj that's the point of this PR:

  • when on netcore2.x -> reference Amazon.Lambda.Logging.AspNetCore version 3.0.0
  • when on netcore3.x -> reference Amazon.Lambda.Logging.AspNetCore version 3.1.0

if there are people on 5, we should of course provide another version for it.

The best would be to split code into branches based on runtime version. We should avoid mixing major versions - if I run netcore3.1 I don't want to see dependency targeting netcore2.1, because it can have side effect elsewhere.

Copy link
Member

Choose a reason for hiding this comment

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

Why would we want .NET Core 2.1 users use the versions that came for .NET Core 3.0? In general by targeting a lower version it will work for versions going forward. For example if you look at the Serilog log provider it still references version 2.0.0 of Microsoft.Extensions.Logging.

We would only need to create a separate build for the newer runtimes if we were using features that were only available in the newer runtimes.

In this case we made a poor decision by using a type that was in an "internal" namespace. And that is the part of the PR I want to take which fixes that.

Copy link
Contributor Author

@wdolek wdolek Jun 13, 2020

Choose a reason for hiding this comment

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

Oh, you are right - all of these are netstandard! I will revert that change and keep only NullScope thingy.

In other hand, what do you think about version range? What happens if my app depends on another package which references different version of one of those packages? (Basically what happened to me with logging)

(Are you ok with updating test dependencies? I kept that change.)

@wdolek wdolek changed the title #679 Reference 3.1.x MS packages, use package version ranges #679 Implement NullScope for netcore 3.1 packages compatibility Jun 23, 2020
@normj normj merged commit f848ee4 into aws:master Jun 25, 2020
@normj
Copy link
Member

normj commented Jun 25, 2020

Everything looks good. I'll work on pushing out new NuGet packages for the change.

@normj
Copy link
Member

normj commented Jun 25, 2020

Version 3.0.1 of Amazon.Lambda.Logging.AspNetCore has been released. And version 5.1.2 of Amazon.Lambda.AspNetCoreServer was released to pick up the new dependency.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants