-
Notifications
You must be signed in to change notification settings - Fork 43
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
Fix #104 - Support PostgreSQL alternative sink #105
Conversation
…/github.com/serilog-contrib/serilog-ui into feature/support-postgresql-sink-alternative
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just added some nit-picker comments to clean it up but the PR is fine and ready for merge!
About the tests, I think we need integration using the Alternative sink but we can add them later, that's fine :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move it inside Postgres...Provider/Models/ folder, just for clarity :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would move it inside Postgres...Provider/Models/ folder, just for clarity :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per previous comment, would move inside Postgres...Provider/Models
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per previous comment, would move it inside Postgres...Provider/Models/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per previous comment, would move it inside Postgres...Provider/Models/
tests/Serilog.Ui.PostgreSqlProvider.Tests/DataProvider/DataProviderBaseTest.cs
Show resolved
Hide resolved
@followynne I applied your comments; please re-review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all good from my side, I only see we miss the namespace change in the 4 Models/ files (from Provider to Provider.Models)
if you can apply that change, then I'd say let's go 😄
This PR aims to fix issue 104, to support Serilog.Sinks.Postgresql.Alternative sink.
Hence, existing integration tests are according to Serilog.Sinks.PostgresSQL, I added some unit tests (
QueryBuilderTests.cs
) for query generation based on Serilog.Sinks.Postgresql.Alternative. Once we refactored integration tests and used sinks to insert log data, we could unify tests for both sinks.