Skip to content

Support MS Config v2, fixes #108, #111, #99 #123

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

Merged
merged 1 commit into from
Jul 9, 2018
Merged

Support MS Config v2, fixes #108, #111, #99 #123

merged 1 commit into from
Jul 9, 2018

Conversation

MV10
Copy link
Contributor

@MV10 MV10 commented May 9, 2018

This is a refresh of PR 120. This involves structural changes to the project, please don't merge other PRs until this one is complete (or withdrawn 😄), it can be quite difficult to merge config-related changes into this PR.

Fixes #108, adds .NET Standard MS Configuration 2.x package support, converts the project to multi-target, and adds ColumnOptions configuration via config section (.NET Standard only). Also fixes #111 and fixes #99, and touches on #91 (see issue comments, maybe shouldn't close that one).

New to this PR:

  • updated README to note minimum Serilog Config NuGet package (3.0.0-dev-00111)
  • net45 build target folder is Configuration\NetFramework.ConfigurationManger
  • netstandard2_0 build target is Configuration\Microsoft.Extensions.Configuration
  • removed a few odd/unnecessary using statements (ex. System.Drawing)
  • per issue Require MaxLength property to be set for DataColumns of type string #91 added exceptions for missing column length on varchar etc.
  • added exception when SQL data type is unrecognized or unsupported.
  • added DataLength and AllowNull to custom columns-by-config and README updated

As in the earlier PR:

  • csproj altered for multi-targeting
  • config-related source moved to build-specific folders
  • net452 references removed (unnecessary, fully backwards-compatible with minimum target net45)
  • unused/unneeded csproj constants (NETCORE, NETSTANDARD, NETSTANDARD2_0) removed
  • package reference tags split between a shared ItemGroup and TFM-conditional ItemGroups
  • moved SQL/.NET data type conversion to ConvertSqlDataType to simplify config extension methods
  • README extensively updated
  • added optional IConfiguration appConfiguration to extension (for named connection string)
  • added optional IConfigurationSection columnOptionsSection parameter on config extension
  • miscellaneous cleanup (zombie using statements, formatting, etc.)
  • unit test csproj altered to support build-target-specific tests
  • added netstandard2_0 unit test for named connection string from IConfiguration
  • added netstandard2_0 unit test for ColumnOptions from IConfigurationSection

@MV10
Copy link
Contributor Author

MV10 commented May 9, 2018

Question for anyone willing to review this monster:

Currently Serilog.Settings.Configuration is case-sensitive, and my key names follow the JSON convention of camel-case. However, just about everything else in Serilog is Pascal-case. My opinion is that Configuration should be case-insensitive, but it sort of bugs me that this bit of config deviates from everything else in Serilog, even if everything else is perhaps wrong(ish) from a JSON-purist standpoint. (Further complicating the question is that JSON is not the only possible source of configuration data...)

Opinions? I'm leaning towards revising to match the rest of Serilog, then go fight the case-insensitivity battle in the Configuration repo...

@MV10
Copy link
Contributor Author

MV10 commented May 20, 2018

Tagging @nblumhardt hoping to get this reviewed or merged -- not sure who has merge capability here.

For the record, I'll be offline May 24-29, then way offline (Cuba!) June 6-12, in case somebody puts time into this one and I seem unresponsive...

@nblumhardt
Copy link
Contributor

Thanks for looping me in, @MV10 - I'm unfortunately not able to loop back around to this just now.

The team with commit access to this repository is @serilog/reviewers-ms-sql-server - anyone there able to take a good look into this one? Cheers, all! :-)

(It doesn't help with this one, unfortunately, but I've added you to that team now, too, Jon.)

@MV10
Copy link
Contributor Author

MV10 commented May 22, 2018

Thanks Nick.

For anyone concerned about getting involved with this one, it probably isn't as dramatic as it seems at first glance. Basically the earlier .NET Framework ConfigurationManager stuff is unchanged, and multi-targeting was added so I could add Microsoft.Extensions.Configuration (v2) support. I get all detailed with the change log and check-ins but that's the Executive Summary of the intent of this PR.

@MV10
Copy link
Contributor Author

MV10 commented Jun 28, 2018

@serilog/reviewers-ms-sql-server ... anybody?

@mivano
Copy link
Contributor

mivano commented Jul 9, 2018

Sorry, I do not have a lot of time to look into this repository and I m also not an active user of this sink at the moment. Maybe it is best to just your own judgment and merge since you are a reviewer?

@MV10 MV10 merged commit b27c618 into serilog-mssql:dev Jul 9, 2018
@MV10 MV10 deleted the ms_config_v2 branch July 9, 2018 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants