Skip to content
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

Configuration during startup #5241

Merged
merged 9 commits into from
Jan 24, 2018
Merged

Configuration during startup #5241

merged 9 commits into from
Jan 24, 2018

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Jan 22, 2018

Fixes #5230

Internal Review Topics:
Startup topic
Configuration topic
Options topic

I found a spot (conventional Startup and convenience startup approaches) in app startup to show this. We already touch the concept with IHostingEnvironment. It fits well here.

@HaoK, especially head-check me on ...

Options can't be accessed during startup without building a service provider, which is an anti-pattern because it requires loading the IOptions service in code.

## Additional notes

* Dependency Injection (DI) is not set up until after `ConfigureServices` is invoked.
* The configuration system is not DI aware.
* `IConfiguration` has two specializations:
* `IConfigurationRoot` Used for the root node. Can trigger a reload.
* `IConfigurationSection` Represents a section of configuration values. The `GetSection` and `GetChildren` methods return an `IConfigurationSection`.
* Use [IConfigurationRoot](https://docs.microsoft.com/ dotnet/api/microsoft.extensions.configuration.iconfigurationroot) when reloading configuration or need access to each provider. Neither of these situations are common.
* Use [IConfigurationRoot](/dotnet/api/microsoft.extensions.configuration.iconfigurationroot) when reloading configuration or need access to each provider. Neither of these situations are common.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need --> needing

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's see if "for" works there.

... I see more "you" and "your" here, too. I may as well 🔪 those while I'm here.

@guardrex
Copy link
Collaborator Author

ty @scottaddie ... Ready for another look 👀.

@HaoK
Copy link
Member

HaoK commented Jan 23, 2018

@guardrex I wouldn't say its an anti-pattern because of service locator. Its more that you are in the midst of configuring options and building the services in ConfigureServices, so there's always an ordering issue, in that if they build a service provider and use the options, it wouldn't contain any configures that are added after they built the service provider.

I would tweak the wording a bit, with the main point being something like

IOptions are fine to use in Configure (since services have been built), but may be in an inconsistent state if manually created from inside of ConfigureServices

@guardrex
Copy link
Collaborator Author

@HaoK

How does this look?

IOptions can be used in Configure, since services are built before the Configure method executes. If a service provider is built in ConfigureServices to access options, it wouldn't contain any options configurations provided after the service provider is built. Therefore, an inconsistent options state may exist due to the ordering of service registrations.

Since options are typically loaded from configuration, configuration can be used in startup in both Configure and ConfigureServices. For examples of using configuration during startup, see the Application startup topic.

@HaoK
Copy link
Member

HaoK commented Jan 23, 2018

Sounds good @guardrex


## In-memory provider and binding to a POCO class

The following sample shows how to use the in-memory provider and bind to a class:

[!code-csharp[Main](index/sample/InMemory/Program.cs)]

Configuration values are returned as strings, but binding enables the construction of objects. Binding allows you to retrieve POCO objects or even entire object graphs.
Configuration values are returned as strings, but binding enables the construction of objects. Binding allows the retrievial of POCO objects or even entire object graphs.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"retrievial" --> "retrieval"

* **Never** store passwords or other sensitive data in configuration provider code or in plain text configuration files. Don't use production secrets in your development or test environments. Specify secrets outside of the project so that they can't be accidentally committed to your repository. Learn more about [working with multiple environments](xref:fundamentals/environments) and managing [safe storage of app secrets during development](xref:security/app-secrets).
* If a colon (`:`) can't be used in environment variables on your system, replace the colon (`:`) with a double-underscore (`__`).
* **Never** store passwords or other sensitive data in configuration provider code or in plain text configuration files. Don't use production secrets in development or test environments. Specify secrets outside of the project so that they can't be accidentally committed to a source code repository. Learn more about [working with multiple environments](xref:fundamentals/environments) and managing [safe storage of app secrets during development](xref:security/app-secrets).
* If a colon (`:`) can't be used in environment variables on a system, replace the colon (`:`) with a double-underscore (`__`).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend adding an example of where the double underscore may be necessary. Maybe we have this documented somewhere already, in which case you could just link to that info.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's in-progress on 👉 #4968

I'll add your comment on the issue referencing this topic.


### GetValue

The following sample demonstrates the [GetValue<T>](/dotnet/api/microsoft.extensions.configuration.configurationbinder.get?view=aspnetcore-2.0#Microsoft_Extensions_Configuration_ConfigurationBinder_Get__1_Microsoft_Extensions_Configuration_IConfiguration_) extension method:

[!code-csharp[Main](index/sample/InMemoryGetValue/Program.cs?highlight=31)]

The ConfigurationBinder's `GetValue<T>` method allows you to specify a default value (80 in the sample). `GetValue<T>` is for simple scenarios and does not bind to entire sections. `GetValue<T>` gets scalar values from `GetSection(key).Value` converted to a specific type.
The ConfigurationBinder's `GetValue<T>` method allows the specification of a default value (80 in the sample). `GetValue<T>` is for simple scenarios and does not bind to entire sections. `GetValue<T>` gets scalar values from `GetSection(key).Value` converted to a specific type.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"does not" --> "doesn't"

@guardrex
Copy link
Collaborator Author

@scottaddie Got 'um 👍

@scottaddie
Copy link
Member

@HaoK Please review one final time.

@HaoK
Copy link
Member

HaoK commented Jan 24, 2018

In the Configuration GetValue sample, there's an extra comment which maybe can be removed, doesn't appear to be any json used in that example

// Requires NuGet package
// Microsoft.Extensions.Configuration.Json

@HaoK
Copy link
Member

HaoK commented Jan 24, 2018

In Startup topic, you don't actually talk about IHostingStartup at all in here or use it in the examples, yet there's a sentence saying "An alternative to injecting IHostingStartup is"

@scottaddie
Copy link
Member

@guardrex Please address Hao's feedback ☝️ and then we'll get this merged.

@guardrex
Copy link
Collaborator Author

@HaoK ty ... great catch ... that bit was supposed to be referring to the use of IHostingEnvironment to base app configuration on the environment ... and going on to say that the dev can use the conventions-based approach if they don't want to set things up that way.

I think I found a 2nd spot where the Requires NuGet package comment can be removed.

@scottaddie scottaddie merged commit 4d17655 into dotnet:master Jan 24, 2018
@guardrex guardrex deleted the guardrex/config-in-configservices branch January 24, 2018 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants