Skip to content

Conversation

@mprice-gcmlp
Copy link
Contributor

Serilog 4.* released without any major breaking changes, this is needed to allow for Serilog sinks to take advantage of the new batching sinks that are native and not in a separate package anymore.

@JoshGlue
Copy link
Contributor

@rnishtala-sumo Could you review this PR?

@emilol
Copy link

emilol commented Jul 15, 2024

@JoshGlue @rnishtala-sumo @francisphn is this one waiting on anything in order to be merged? Is there anything that can be done to help?

@LarsHolm-KMD
Copy link

It's been a few weeks without any activity. We would also like to have this merged.

@JoshGlue
Copy link
Contributor

The previous PR took three months to upgrade Serilog from version 2.x to 3.x (#119). This current PR is also taking a significant amount of time to be merged. Could we consider setting the versioning like this?

<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard2.0' ">
    <PackageReference Include="Serilog" Version="[2.0,)" />
</ItemGroup>

This approach would allow us to update the Serilog package without being restricted by the SumoLogic.Logging.Serilog package. Since tests are run with every PR, the CI pipeline would catch any breaking changes in future Serilog versions.

Don't set max major version for Serilog dependency
@mprice-gcmlp
Copy link
Contributor Author

@JoshGlue good call, updated that.

[Insert Jeopardy Theme Music Here]

</ItemGroup>
<ItemGroup Condition=" '$(TargetFramework)' == 'netstandard2.0' ">
<PackageReference Include="Serilog" Version="[2.0,4.0)" />
<PackageReference Include="Serilog" Version="[2.0,)" />

Choose a reason for hiding this comment

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

I don't have enough context here, if this is bumping up to allow Serilog 4.*, why is the change removing 4.0?

Choose a reason for hiding this comment

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

The PR removes the requirement for at specific version of Serilog, except above version 2.0.
Basically the parentesens and braces indicate "including or not", so before they meant above 2.0, but below 4.0. This means you could install 2.2.x, 3.x.x, but no 4.x.x.
Removing the 4.0 from the PackageReference just means 2.0 and above.

Choose a reason for hiding this comment

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

Ah gotcha, didn't noticed the bracket and parenthesis. Makes alot more sense, approved!

@cssatkmd
Copy link

Who can merge this PR?

@rnishtala-sumo rnishtala-sumo merged commit a30ed9c into SumoLogic:main Aug 21, 2024
@cssatkmd
Copy link

When can we expect a new package to be available?

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.

8 participants