SqlConnection config action #613
Conversation
nhart12
force-pushed
the
sqlConnectionConfiguration
branch
2 times, most recently
from
7b1cd54 to
3706077
Compare
Signed-off-by: Nathan Hart <[email protected]>
nhart12
force-pushed
the
sqlConnectionConfiguration
branch
from
3706077 to
b1b22c4
Compare
|
When do you typically update Microsoft.Data.SqlClient? Any reason we can't take latest? |
SqlClient v6.x has breaking changes. Such as dropping support for netstandard 2.0 and Sql Server 2014. I think we should wait for 5.2.3. |
Gotcha. Any reason why this library needs to continue to support .netstandard even though SqlClient itself doesn't and .netstandard shouldn't be required since it is already mulit-targetting for various .net frameworks? |
|
@nhart12 Serilog core still does support .NET Standard (https://github.com/serilog/serilog/blob/dev/src/Serilog/Serilog.csproj) so I'm hesitant to remove it from MSSQL sink. Once they remove it, we should be safe to remove it too. Which features from SqlClient 6 do you need? Btw: Your PR change from SqlConnectionFactory to SqlConnectionConfiguration action looks pretty good. I already wanted to suggest you change it that way. Apparently we had the same thought :). If you add some tests and document the new sink option in the README, the PR can be merged. |
|
Makes sense to wait for Serilog itself. I'm dependent on dotnet/SqlClient#2253 which I'm not sure what versions it will get into. |
|
@ckadluba I added a couple of very basic tests, let me know if you'd like something more specific |
|
Thanks @nhart12! Please also add a short description about the new sink option in the README. Although it should be clear: still mention maybe that this sink option, unlike the others, cannot be read from a config file or other config sources. Once documented this PR is good to go from my point of view. :) |
|
Okay, README is already done from your side, so forget my last comment. 😅 PR can be merged. |
|
PR merged and a prerelease 8.1.2-dev-00126 was published to nuget.org. @nhart12 can you please give this one a spin and let me know the results? If everything works as expected we can create a 8.2.0 stable release. Thank you for the nice contribution! :) |
@ckadluba Just gave it a spin, and the hook is indeed called! Thank you for merging! |
Based on feedback from #609.. Came up with another approach which Closes #608..