Add the ConfigurationManager to the IServiceCollection #36832
Conversation
- Add an IConfiguration to the service collection for scenarios where uses end up prematurely building the service provider or sniffing the IServiceCollection for it. We then remove the ConfigurationManager reference it from the final IServiceCollection to avoid cycles in the configuration graph that result in stack overflows (we link the configuration manager to the final configuration that has been built). - Added tests
ghost
added
the
| var app = builder.Build(); | ||
|
|
||
| // These are different | ||
| Assert.NotSame(app.Configuration, builder.Configuration); |
There was a problem hiding this comment.
This is kinda annoying, no? Can we keep the builder.Configuration registration if it's added manually?
There was a problem hiding this comment.
I can make WebApplication.Configuration and WebApplicationBuilder.Configuration match, but I can't make them match the final IConfiguration instance. The problem is that we need to chain the ConfigurationManager to the final IConfiguration so we can see the providers added by unit tests. There's no way to get the IConfiguration from the built application other than via the IServiceProvider.
|
cc @dotnet/aspnet-build This was approved. Also cc @Pilchie |
|
Just curious: Is this a backport from 'main'❔ Our current approach is supposedly to start all fixes in 'main' |
|
Should also mention this fix will not reach 'main' unless you do or have done something manual |
|
/backport to main |
|
Started backporting to main: https://github.com/dotnet/aspnetcore/actions/runs/1263698985 |
|
@davidfowl an error occurred while backporting to main, please check the run log for details! Error: @davidfowl is not a repo collaborator, backporting is not allowed. |
|
I don't think you can run backport from a branch you don't have permission to merge from. |
|
😮 |
|
The failures to backport is a GitHub bug affecting many members of our team with |
|
/backport to main |
|
Started backporting to main: https://github.com/dotnet/aspnetcore/actions/runs/1263724792 |
|
Wow, I can merge to this branch as well as the target branch and have admin rights (through a team). Wonder how @SteveSandersonMS was able to backport from 'main' to 'release/6.0' in #36839❔ /cc @TanayParikh -- more fodder for your ticket @MattGal do you have a direct GitHub contact. Our "backport bot" isn't working much right now even though we haven't changed it in months. See https://github.com/dotnet/aspnetcore/blob/main/.github/workflows/backport.yml @ViktorHofer @Anipik @ericstj @jeffhandley is the runtime team also seeing issues w/ your very similar backport GitHub action❔ (@akoeplinger is on vacation) |
Thanks, added the note and requested escalation on the support ticket. |
Sadly not, the folks I talk with mostly are technically ADO, though that's "GitHub". I'd ask @mmitche / @chcosta maybe? |
- Add an IConfiguration to the service collection for scenarios where uses end up prematurely building the service provider or sniffing the IServiceCollection for it. We then remove the ConfigurationManager reference it from the final IServiceCollection to avoid cycles in the configuration graph that result in stack overflows (we link the configuration manager to the final configuration that has been built). - Added tests
- Add an IConfiguration to the service collection for scenarios where uses end up prematurely building the service provider or sniffing the IServiceCollection for it. We then remove the ConfigurationManager reference it from the final IServiceCollection to avoid cycles in the configuration graph that result in stack overflows (we link the configuration manager to the final configuration that has been built). - Added tests
|
I'm back from vacation 😄 It looks like this was caused by a change on GitHub's end, according to this thread it was reverted so we shouldn't be hitting this issue anymore: https://github.community/t/github-api-collaborators-access-changed-today/201259/14 |
|
@dougbu @davidfowl @TanayParikh @MattGal so I just figured out the root cause of this issue: the auth token used by the GitHub Actions backport job apparently can't see you're a contributor if your membership visibility in the You can go to https://github.com/orgs/dotnet/people, search for your username and select Public in the dropdown:
You can confirm it worked by going to your https://github.com/username profile in an incognito window and see if the purple .NET org logo shows up there (if you look at a user while signed in to your account it will show the org membership regardless of the setting). |
|
@akoeplinger makes sense, the dependency flow failure tagging model has the same problem because the permissions to know the membership when it's private are nigh on org-admin permissions and we avoid our bots having that. |
|
Hi @MattGal. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
|
Is there any downside to making our memberships |
|
/backport to main |
|
Started backporting to main: https://github.com/dotnet/aspnetcore/actions/runs/1426679369 |
|
Just validating, will immediately delete the PR. |
|
@TanayParikh backporting to main failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Add the ConfigurationManager to the IServiceCollection - Add an IConfiguration to the service collection for scenarios where uses end up prematurely building the service provider or sniffing the IServiceCollection for it. We then remove the ConfigurationManager reference it from the final IServiceCollection to avoid cycles in the configuration graph that result in stack overflows (we link the configuration manager to the final configuration that has been built). - Added tests
Using index info to reconstruct a base tree...
M src/DefaultBuilder/src/WebApplicationBuilder.cs
M src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/WebApplicationTests.cs
Falling back to patching base and 3-way merge...
Auto-merging src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/WebApplicationTests.cs
CONFLICT (content): Merge conflict in src/DefaultBuilder/test/Microsoft.AspNetCore.Tests/WebApplicationTests.cs
Auto-merging src/DefaultBuilder/src/WebApplicationBuilder.cs
CONFLICT (content): Merge conflict in src/DefaultBuilder/src/WebApplicationBuilder.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Add the ConfigurationManager to the IServiceCollection - Add an IConfiguration to the service collection for scenarios where uses end up prematurely building the service provider or sniffing the IServiceCollection for it. We then remove the ConfigurationManager reference it from the final IServiceCollection to avoid cycles in the configuration graph that result in stack overflows (we link the configuration manager to the final configuration that has been built). - Added tests
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
Okay great, I think that did it! Thanks @akoeplinger! |
That's just the default. I suppose it may make you potentially the target of advertising / recruiting / worse things, beyond that I don't think there's a downside. |
|
Hi @MattGal. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context. |
amcasey
added
area-minimal
Fixes #36829
Description
Add an
IConfigurationto the service collection for scenarios where uses end up prematurely building the service provider or sniffing theIServiceCollectionfor it. We then remove theConfigurationManagerreference it from the finalIServiceCollectionto avoid cycles in the configuration graph that result in stack overflows (we link the configuration manager to the final configuration that has been built).Customer Impact
This can affect libraries that try to access the service provider before the bootstrapping of the application is complete. This should be identical between .NET 5 and .NET 6 but missing the
IConfigurationat this early stage has broken a customer's library.Regression
Yes
Testing
Added unit tests and verified customer scenario.
Risk
Low