- IdentityServer4, Admin, Admin.Api, STS.Identity, added support for environment-specific configuration files #551

ViRuSTriNiTy · 2020-03-25T09:54:07Z

… in logging setup and CreateHostBuilder(), see issue #549

…environment-specific configuration files in logging setup and CreateHostBuilder(), refs issue skoruba#549

…endings to clean-up PR, refs issue skoruba#549

…8 BOM in modified files to clean-up PR, refs issue skoruba#549

b0 · 2020-03-25T10:26:27Z

src/Skoruba.IdentityServer4.Admin.Api/Program.cs

@@ -38,7 +38,9 @@ private static IConfiguration GetConfiguration(string[] args)
            var configurationBuilder = new ConfigurationBuilder()
                .SetBasePath(Directory.GetCurrentDirectory())
                .AddJsonFile("appsettings.json", optional: false, reloadOnChange: true)
-                .AddJsonFile("serilog.json", optional: true, reloadOnChange: true);
+                .AddJsonFile($"appsettings.{environment}.json", optional: true, reloadOnChange: true)


Do we really need this line ? I think apps already works with appsettings{environemnt}.json by default.

Imho yes, this line is necessary as there is no "automagic" going on here, see #549 (comment)

I don't think we need this line. If anyone wants a custom serilog configuration. The configuration should be mounted into the application.

Define in your release pipeline to mount the serilog.json from the outside to the inside. The proposed line will not work with docker and kubernetes

I don't think we need this line. If anyone wants a custom serilog configuration. The configuration should be mounted into the application.

Define in your release pipeline to mount the serilog.json from the outside to the inside. The proposed line will not work with docker and kubernetes

Thanks for your feedback but i don't understand your instructions. What does "mounted into the application" actually mean and what "release pipeline"? Do you mean i should copy serilog.Development.json to serilog.json before releasing the application? And why is the proposed solution not working with docker and kubernetes?

if you look at the docker compose below
https://github.com/skoruba/IdentityServer4.Admin/blob/master/docker-compose.yml#L35

This give the flexibility for anyone to define serilog.json as they want and mount it from the host machine to the container

To me it is not necessary but if you want I'd say if you still want to do it. do it in your fork

So you want only "necessary" changes not improvements, that's sad as my approach also allows config transformations via SlowCheetah, all effort in vain. Closing the PR.

I am not the owner of the repo. I just gave a comment.

The owner can very well decide to approve your pr

b0 · 2020-03-25T10:26:40Z

src/Skoruba.IdentityServer4.Admin/Program.cs

@@ -61,7 +61,9 @@ private static IConfiguration GetConfiguration(string[] args)
            var configurationBuilder = new ConfigurationBuilder()
                .SetBasePath(Directory.GetCurrentDirectory())
                .AddJsonFile("appsettings.json", optional: false, reloadOnChange: true)
-                .AddJsonFile("serilog.json", optional: true, reloadOnChange: true);
+                .AddJsonFile($"appsettings.{environment}.json", optional: true, reloadOnChange: true)


Do we really need this line ? I think apps already works with appsettings{environemnt}.json by default.

Imho yes, see https://github.com/skoruba/IdentityServer4.Admin/pull/551/files#r397750808

b0 · 2020-03-25T10:32:19Z

At least how I am reading this: https://docs.microsoft.com/en-us/aspnet/core/fundamentals/configuration/?view=aspnetcore-3.1#default-configuration

appsettings.json and appsettings.{environment}.json are "magicaly" provided by default.

Best regards

ViRuSTriNiTy · 2020-03-25T10:55:57Z

@bo Yes i read this section too but i think its a bit misleading. I've checked the .NET Core sources and as i mentioned in the related issue the appsettings.{environment}.json is added by the DefaultBuilder, see https://github.com/dotnet/aspnetcore/blob/master/src/DefaultBuilder/src/WebHost.cs#L170. The issue here is that the logging configuration is created without the DefaultBuilder hence you need to do it manually.

Edit

I've double checked this again via digging in the sources and by testing it myself with a simple breakpoint, see:

In the screenshot you can see that only appsettings.json and serilog.json is loaded via the JSONConfigurationProvider which is added by AddJsonFile(). The additional EnvironmentVariablesConfigurationProvider just provides values from environment variables not from an environment based configuration file like appsettings.{environment}.json as you can see in the implementation details, see https://github.com/dotnet/extensions/blob/master/src/Configuration/Config.EnvironmentVariables/src/EnvironmentVariablesConfigurationProvider.cs.

Hope this helps to understand the issue.

b0 · 2020-03-25T23:10:58Z

Hi @ViRuSTriNiTy ,

indeed, logging configuration is created differently and those lines are needed.

Great work. 👍

Thank you

ViRuSTriNiTy · 2020-03-26T00:45:44Z

@b0 Thanks for reviewing. How to remove the merge block? Is @skoruba needed to resolve this?

ViRuSTriNiTy · 2020-04-08T07:29:16Z

@skoruba Please unblock the PR when everything is ok.

ViRuSTriNiTy · 2020-04-14T09:06:47Z

PR closed as a result of #551 (comment)

skoruba · 2020-04-14T09:20:16Z

@ViRuSTriNiTy - please, reopen this PR. I want to use it.
Thanks.

ViRuSTriNiTy · 2020-04-14T09:21:30Z

@skoruba Reopened as requested

skoruba · 2020-04-18T08:11:03Z

Thanks 👍🏼

ViRuSTriNiTy added 3 commits March 25, 2020 10:50

- IdentityServer4, Admin, Admin.Api, STS.Identity, added support for …

197e098

…environment-specific configuration files in logging setup and CreateHostBuilder(), refs issue skoruba#549

- IdentityServer4, Admin, Admin.Api, STS.Identity, fixed end of line …

efd9af5

…endings to clean-up PR, refs issue skoruba#549

- IdentityServer4, Admin, Admin.Api, STS.Identity, fixed missing UTF-…

acd7098

…8 BOM in modified files to clean-up PR, refs issue skoruba#549

ViRuSTriNiTy marked this pull request as ready for review March 25, 2020 10:05

b0 reviewed Mar 25, 2020

View reviewed changes

ViRuSTriNiTy closed this Apr 14, 2020

ViRuSTriNiTy reopened this Apr 14, 2020

skoruba approved these changes Apr 18, 2020

View reviewed changes

skoruba merged commit f92c0dd into skoruba:dev Apr 18, 2020

skoruba mentioned this pull request Jul 22, 2020

1.0.0-RC2 #658

Merged

Uh oh!

- IdentityServer4, Admin, Admin.Api, STS.Identity, added support for environment-specific configuration files #551

- IdentityServer4, Admin, Admin.Api, STS.Identity, added support for environment-specific configuration files #551

Uh oh!

Conversation

ViRuSTriNiTy commented Mar 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViRuSTriNiTy Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ViRuSTriNiTy Apr 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b0 commented Mar 25, 2020

Uh oh!

ViRuSTriNiTy commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

b0 commented Mar 25, 2020

Uh oh!

ViRuSTriNiTy commented Mar 26, 2020

Uh oh!

ViRuSTriNiTy commented Apr 8, 2020

Uh oh!

ViRuSTriNiTy commented Apr 14, 2020

Uh oh!

skoruba commented Apr 14, 2020

Uh oh!

ViRuSTriNiTy commented Apr 14, 2020

Uh oh!

skoruba commented Apr 18, 2020

Uh oh!

Uh oh!

ViRuSTriNiTy Mar 25, 2020 •

edited

Loading

ViRuSTriNiTy Apr 13, 2020 •

edited

Loading

ViRuSTriNiTy commented Mar 25, 2020 •

edited

Loading