twilio-labs / NgrokExtensions

Visual Studio integration with ngrok
MIT License
48 stars 26 forks source link

Add ability to configure settings in secrets.json file #16

Closed ChristopherHaws closed 6 years ago

ChristopherHaws commented 6 years ago

Hello,

This PR adds support for overriding settings in appsettings.json with settings in secrets.json as described in https://github.com/dprothero/NgrokExtensions/issues/15.

I was going to write some unit tests but I would have had to build mock objects for a bunch of the DTE objects (which is what projects like StyleCop ended up doing: https://github.com/StyleCop/StyleCop/tree/master/Project/Test/Tests.StyleCop.VisualStudio/Mocks).

dprothero commented 6 years ago

@ChristopherHaws so stoked to see another community PR! I definitely appreciate this use case, and this could be one way to solve it. It occurred to me to think about how I solve this similar issue in other situations... I tend to use environment variables. Perhaps an environment variable would be the appropriate solution here? WDYT?

Also, I'm not sure why the build is failing. Will have to investigate that.

ChristopherHaws commented 6 years ago

Well, secrets and environment variables are treated separately in asp.net core configuration (secrets take precedence over env variables). The reason for this is that env variables are loaded before json files, and then secrets are loaded last. What this means is that secrets override json.env, json.env overrides json, and json overrides env variables.

It would be really nice to simply use Microsoft.Extensions.Configuration to handle this, but I don't know that there would be an easy way to ask the application for the order they are configured, since this is done in code.

We could assume that users are using ASP.NET Core's default setup which is something like:

var builder = new ConfigurationBuilder()
    .SetBasePath(env.ContentRootPath)
    .AddEnvironmentVariables()
    .AddJsonFile("appsettings.json", optional: true, reloadOnChange: true)
    .AddJsonFile($"appsettings.{env.EnvironmentName}.json", optional: true)
    .AddSecrets();

var configuration = builder.Build();

I wonder if we could do something similar to what EF Core is doing in their migration tools. They actually take the compiled dll for your app and call ConfigureServices in your startup class to get the IServiceProvider so that they can resolve the DbContext. The issue here is that config is setup as part of the IWebHostBuilder so I don't think there is a good way to do this.

dprothero commented 6 years ago

I'm actually questioning whether this setting is really a project-level setting at all. Perhaps it should just be a setting for the extension.

ChristopherHaws commented 6 years ago

I'm not so sure about that. We have several projects that we might want to have different subdomains setup for. I think the better option would be to actually bake it into csproj.

<PropertyGroup>
    <NgrokScheme>http</NgrokScheme>
    <NgrokPort>80</NgrokPort>
    <NgrokSubdomain>my-subdomain</NgrokSubdomain>
</PropertyGroup>

This would be nice since then we could make a dotnet command for dotnet ngrok or something along those lines allowing this functionality to be platform agnostic.

dprothero commented 6 years ago

That sounds great! I'm going to merge this PR (once I fix the build) since I believe it would still be useful. If you'd like to submit another PR with the csproj, I'll happily review and merge!

dprothero commented 6 years ago

I need to push some changes to fix the build (update Microsoft.VSSDK.BuildTools), so I am closing this PR and will open another one.