-
Notifications
You must be signed in to change notification settings - Fork 15
Make configuration work independent of whether appsettings json files are provided #337
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
Make configuration work independent of whether appsettings json files are provided #337
Conversation
|
@ChrisDoernen You're welcome! Right on time! I was thinking making the same change too! Please give me a couple of days to review this PR. There are some other projects heavily relying on Can you please check something else? I remember there is a feature to accommodate the development machine's secret settings/values. Can you please verify if that feature is not broken after your change in this PR? Thanks. |
|
Nice! Yes sure, take your time. I did not yet use this feature, do you mean overriding |
…assert the secrets are available in the application via configuration.
…s' into get-configuration-root-regardless
|
@Arash-Sabet ok I got what you mean, check out my second commit. I am setting the (randomized) secrets in the test and assert that they are available. However, the secrets remain set after the test did run. For me it is not a problem and the randomization ensures no collisions. If you want me to do a clean up, I am happy to implement it. |
|
@ChrisDoernen I left a few comments in this PR and you can review them in the "File View" section. Resolving them paves the path for my next step. |
|
@Arash-Sabet I can not find the "File View" section and your comments. Can you post a screenshot where to click? I expected to view comments in the "Files Changed" tab:
|
|
@ChrisDoernen Does not vs code display my comments if you open the changed files in it? It should I guess. |
|
I am working in JetBrains Rider... |
|
@ChrisDoernen Please see the screenshots below:
|
…d. Then assert the secrets are available in the application via configuration." This reverts commit b4dc253.
|
@Arash-Sabet I reverted my second commit and made a new one with an explicit test case for configuration without appsettings. All tests are green. |
|
@Arash-Sabet what do you think? TestBedFixture line 153 is the main change: We build the configuration always, not only when appsettings are used. This enables using env variables only (without appsettings). |
|
@ChrisDoernen Thanks. I will review it soon. |
Bump .NET SDK version to 10.0.101 and increment patch/revision numbers in both Azure pipeline YAML files. Remove unnecessary blank lines in ConfigurationTestsWithoutAppsettings.cs for cleaner test code.
Update GetConfigurationRoot to ignore TestAppSettings entries with null or empty filenames when adding JSON files to the configuration builder.
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.
@ChrisDoernen Can you please restore the original implementation of ConfigurationTersts class and instead add a new configuration tests class to verify TestProjectFixtureWithoutAppsettings's implementation?
| private IConfigurationRoot GetConfigurationRoot() | ||
| { | ||
| var testAppSettings = GetTestAppSettings(); | ||
| return |
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.
@ChrisDoernen what is the reason for removing the condition?
| { | ||
| "Options": { | ||
| "Rate": 10 | ||
| }, |
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.
Hey @ChrisDoernen we did not need to remove anything or alter anything related to using secret values. Can you please undo this change?
Let's put it in this way: I will only accept this PR if the changes are incremental. I do not want to get the already-implemented feature excluded or removed unless they are obsolete.
| { | ||
| public string? Secret1 { get; set; } | ||
| public string? Secret2 { get; set; } | ||
| public required string Secret1 { get; set; } |
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.
@ChrisDoernen the required keyword is unnecessary.
| public void TestSecretValues() | ||
| { | ||
| /* | ||
| * TODO: Create a user secret entry like the following payload in user secrets and remove the same from appsettings.json file: |
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.
@ChrisDoernen Please restore this block of comment
| { | ||
| SetSecret(nameof(SecretValues.Secret1), _secret1Value); | ||
| SetSecret(nameof(SecretValues.Secret2), _secret2Value); | ||
| } |
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.
@ChrisDoernen Please remove this block.
| Assert.Equal(secretValues.Secret2, _secret2Value); | ||
| } | ||
|
|
||
| private void SetSecret(string secretName, string secretValue) |
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.
@ChrisDoernen Unnecessary.
|
Hey @Arash-Sabet, thanks so much for your work. I think it is a useful improvement, I will used it right away! Happy holidays to you and your family! |





Hi @Arash-Sabet,
It is me again☺️ Thanks for merging so quick! I updated to the new version and have a follow-up suggestion.
Currently the configuration is only getting built when there are appsetting files configured (
GetTestAppSettings). We often have the case that our configuration is only coming from environment variables without appsettings file.So the idea is if we can actually build the configuration regardless. This also has the advantage that the
IConfigurationdoes not need to be nullable inAddServices. This makes it a bit nicer for the user because they don't need to check fornull.I also changed
GetTestAppSettingsto be virtual with a default implementation of an empty enumerable, so users don't need to override the method when they don't want to provide appsettings. This would then work likeAddUserSecretes.I adopted the configuration tests (see
TestProjectFixtureWithoutAppsettings), all test are still green.All the best
Chris