-
Notifications
You must be signed in to change notification settings - Fork 10.3k
Apply hosting environment via command line args #34794
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
Conversation
- This makes sure that the entire applicaton can see changes to host configuration (env, content root etc). - Added tests to verify those scenarios
{ | ||
var config = new[] | ||
{ | ||
KeyValuePair.Create("Greeting", "Bonjour tout le monde"), |
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.
You kept it 😄
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.
I'm not familiar enough with this area to sign off. @halter73 could you look at this?
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. |
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.
Undo
// Transform the host configuration into command line arguments | ||
foreach (var (key, value) in _hostConfiguration.AsEnumerable()) | ||
{ | ||
args.Add($"--{key}={value}"); |
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.
Do we need to do any fancy escaping for value
? Like can values have spaces which would break config parsing? I'm not super familiar with this area
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.
I don't see how it breaks given how the parsing works. We're not parsing the command line string, we're parsing each arg.
I also don't see a way to escape -
or /
so that might be the only problem.
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.
An =
in the key would also mess this up, right? I'm not sure how realistic that is.
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.
I also don't see a way to escape - or / so that might be the only problem.
You could always base64 encode/decode. Stripping and adding back the =
padding characters might be a pain though.
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.
I think we wait until somebody hits this then we fix it by supporting escape sequences in the command line configuration provider, the problem is general.
Fixes #33889