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

Truncate additional columns, allow varchar on standard columns #490

Merged
merged 4 commits into from
Nov 20, 2023

Conversation

nhart12
Copy link
Contributor

@nhart12 nhart12 commented Nov 13, 2023

Fix for #489 & #436

Allows setting varchar for standard columns of string types.
Auto-truncate string type additional columns to the datalength

@ckadluba
Copy link
Member

Hi @nhart12!

Thank you for your contribution. We love good and useful additions to our project, especially because we ourselves have only limited time to work on the sink. Therefore quality contributions are always welcome.

I will be able to take a closer look at the PR in some days (probably on the weekend).

In the meantime, could you please add/update description of the new feature in README.md?

Thanks,
Christian

@nhart12
Copy link
Contributor Author

nhart12 commented Nov 18, 2023

Thank you, good suggestions, I think I've addressed them all in the latest

@ckadluba ckadluba enabled auto-merge November 18, 2023 20:24
@ckadluba
Copy link
Member

Thank you for the fixes.

There seems to be an error due to a language feature used in SqlTypes.cs which is not available in C# 7. The sink cannot use a newer language version due to support of (old) .Net Framework.

Can you try to fix this?

auto-merge was automatically disabled November 19, 2023 00:33

Head branch was pushed to by a user without write access

@nhart12
Copy link
Contributor Author

nhart12 commented Nov 19, 2023

Sure I think I found the issue

@ckadluba
Copy link
Member

ckadluba commented Nov 19, 2023

There is a problem now with a Newtonsoft dependency during test execution. I don't think this is related to your PR. I will take a look at this as soon as I can (maybe tomorrow).

@ckadluba
Copy link
Member

Hi there!

I created bug #491 for this problem.

The reason for the .NET 6 test run to fail is that the package Microsoft.NET.Test.Sdk had to be added to the Serilog.Sinks.MSSqlServer.Tests project. I did this and merged it into the dev branch.

Please rebase your branch onto dev and this PR should be able to go forward.

@nhart12
Copy link
Contributor Author

nhart12 commented Nov 20, 2023

Thank you, I just rebased so hopefully tests run ok now

@ckadluba ckadluba added this pull request to the merge queue Nov 20, 2023
Merged via the queue into serilog-mssql:dev with commit 480d36b Nov 20, 2023
@ckadluba
Copy link
Member

@nhart12 congratulations and a big thank you! Your PR was merged to dev branch and version 6.3.0-dev-00055 of the MSSQL sink was created and published on nuget.org. :)

Can you please test your contribution with that version and let us know if you find any problem?

Thank you very much!
Christian

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.

2 participants