Skip to content
This repository has been archived by the owner on Oct 28, 2024. It is now read-only.

appsettings.json and Proxies/appsettings.json should be one file #180

Closed
bacongobbler opened this issue Jul 23, 2021 · 6 comments
Closed
Labels
enhancement New feature or request

Comments

@bacongobbler
Copy link
Contributor

bacongobbler commented Jul 23, 2021

configuration for hippo and for YARP should be controlled through one settings file. YARP configuration can be namespaced under a Proxies namespace or other setting. But a user should not have to maintain two separate "settings" file for each environment to configure the same program.

https://github.com/deislabs/hippo/blob/main/Hippo/appsettings.json
https://github.com/deislabs/hippo/blob/main/Hippo/Proxies/appsettings.json

@bacongobbler bacongobbler added the enhancement New feature or request label Jul 27, 2021
@simongdavies
Copy link
Member

This is intentional, if we use the same configuration files then there is no way to have Kestrel listening on different endpoints for the proxy and hippo. In fact, I think that merging the configuration will throw an exception as I suspect both hippo and the proxy will try and listen on the same endpoint

@bacongobbler
Copy link
Contributor Author

bacongobbler commented Jul 29, 2021

Why can't each HostBuilder use one file with the kestrel config namespaced under Kestrel.Hippo (or Hippo.Kestrel) and Kestrel.Proxies? Is Kestrel's configuration hard-coded in the JSON document?

I thought ASP.NET Core's best practices demonstrated one Program.cs == one appsettings file for each environment, such that appsettings.Development.json would configure your application if you're in development, and appsettings.json for production.

Right now it looks like we have multple appsettings.Development.json files: one for each HostBuilder class. So if I want to deploy Hippo to development with the WAGI.NET scheduler in #182, it looks like I'll need THREE appsettings.Development.json files to tweak Hippo's settings: one for Hippo, one for YARP, and one for WAGI.NET.

isn't that a bit confusing to the end user?

@simongdavies
Copy link
Member

Why can't each HostBuilder use one file with the kestrel config namespaced under Kestrel.Hippo (or Hippo.Kestrel) and Kestrel.Proxies? Is Kestrel's configuration hard-coded in the JSON document?

Kestrel will look for its configuration under the Kestrel Node in configuration, I dont think that there is a way to change this but I will check , its possible that it can be done through an option.

Right now it looks like we have multple appsettings.Development.json files: one for each HostBuilder class. So if I want to deploy Hippo to development with the WAGI.NET scheduler in #182, it looks like I'll need THREE appsettings.Development.json files to tweak Hippo's settings: one for Hippo, one for YARP, and one for WAGI.NET.

One thing that we could do is to have a common base (e.g. appsettings.json, appsettings.json and then have a appsettingsHippo.json , appsettingsYarp.json these files could be used only for the specific config for those webhosts and we could place them in the same directory.

@simongdavies
Copy link
Member

In addition I don't think that wagi-dotnet actually needs any user defined configuration so we can probably delete those files and point it at the hippo configs.

@simongdavies
Copy link
Member

Kestrel will look for its configuration under the Kestrel Node in configuration, I dont think that there is a way to change this but I will check , its possible that it can be done through an option.

IConfiguration can be passed via KestrelSeverOptions, so we could have something like:

"Hippo" : {
  "Kestrel": {
    "Endpoints": {
      "Http": {
        "Url": "http://localhost:5000"
      },
      "Https": {
         "Url": "https://localhost:5001"
      }
    }
  },
},
"Yarp" : {
  "Kestrel": {
    "Endpoints": {
      "Http": {
        "Url": "http://localhost:5002"
      },
      "Https": {
         "Url": "https://localhost:5003"
      }
    }
  },
}

Then we would read the relevant section and configure Kestrel via options.
Just need to check that Yarp will pick this up if we do it this way.

@bacongobbler bacongobbler linked a pull request Dec 2, 2021 that will close this issue
@bacongobbler
Copy link
Contributor Author

Fixed in #311

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants