-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Installer connection string, default to appsettings.Development.json #20321
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
base: main
Are you sure you want to change the base?
Installer connection string, default to appsettings.Development.json #20321
Conversation
|
Hi there @callumbwhyte, thank you for this contribution! 👍 While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:
Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution. If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request. Thanks, from your friendly Umbraco GitHub bot 🤖 🙂 |
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.
Pull Request Overview
This PR modifies Umbraco's installer to prioritize storing connection strings in appsettings.Development.json over appsettings.json for better development practices. The change addresses the issue where connection strings were always placed in the base configuration file, which could cause deployment issues when local SQLite databases aren't available in production environments.
Key changes:
- Installer now checks for
appsettings.Development.jsonfirst and uses it for connection string storage if available - Falls back to
appsettings.jsonif the development-specific file doesn't exist - Updated the configuration provider selection logic to filter by environment-specific file names
src/Umbraco.Infrastructure/Configuration/JsonConfigManipulator.cs
Outdated
Show resolved
Hide resolved
|
Hi Callum, thank you for the well thought out PR, I agree with your frustrations here, and have raised it with the HQ team to get some answers :) |
|
I agree with this idea totally and suggested solution looks great. The only time I can see it being an issue is maybe on Cloud when there is just a live environment, so the ASPNETCORE_ENVIRONMENT usage maybe needed there as you mentioned |
|
The database connection string should indeed be environment-specific, but I would argue it shouldn't be limited to the Development environment indeed. Using the current environment from There's also the Umbraco-CMS/src/Umbraco.Web.Common/Hosting/HostBuilderExtensions.cs Lines 24 to 31 in 16c0de8
If this part is moved to the |
Background
It has always frustrated me that Umbraco's installer defaults to putting a connection string in appsettings.json.
The .NET template ships a appsettings.Development.json file by default, and best practise would say this is where you should put your local development config.
.NET resolves configuration top down:
appsettings.jsonappsettings.{Environment}.jsonConfig in
appsettings.Development.jsonis used above config inappsettings.json, etc.Having connection strings in
appsettings.jsondoes actually present some potential issues when deploying beyond local...The connection string may not be accessible on all environments – leading to a database unavailable error on boot. SQLite files are explicitly ignored in gitignore so won't be deployed either...
By default Umbraco installs a SQLite database with the corresponding
umbracoDbDSN_providerNamealso set to use SQLite. Even if an environment specific connection string is configured, it is essential to also set theumbracoDbDSN_providerNametoo else the value will fallback to using what is set inappsettings.json. If the database type doesn't match (likely, don't want to use SQLite in prod) this leads to some confusing boot errors. Whilst setting the providerName is best practise and in the docs, it's not a totally obvious thing to remember IMO.Change
I have adjusted the installer process to first check for the presence of a
appsettings.Development.jsonfile, and to use that instead to store the connection string. If it does not exist theappsettings.jsonfile will be used instead, as currently.When looping to find a
JsonConfigurationProviderinstance, we also check if the configuration file name contains "appsettings.{environment}.json".An additional optional param was added to an already private method, and the order of those params was changed, so all uses of that method have been updated.
Testing
application.Development.jsoninstead ofappsettings.jsonQuestions
Are we happy with the hardcoded
Developmentenvironment name? This is the default that .NET uses, and what Umbraco ships inside launch.json too, so feels sensible. We could default to attempting the currentASPNETCORE_ENVIRONMENTinstead though...?Should there still be a something in
appsettings.json? One benefit of the current approach is there will always be a connection string for every environment, even if it's invalid. This prevents the installer from being able to be run, e.g. in production. (the impact of this feels low... someone would only be able to create a SQLite file) One solution might be to have the installer place a blank connection string inappsettings.jsonas well.Happy to take the guidance on best approach from HQ here!