Experiment to use Microsoft.Extensions.Configuration #306
Conversation
…roll configuration json file and allow for Environment Variable Overrides.
|
This seems like a reasonable way to leverage the MS Configuration components. It does unfortunately highlight the problem of having a "JSON" configuration type as we're loading that information from multiple sources. |
|
@gasparnagy in researching this configuration stuff, I see in a few places a couple of data structures that have the [Serializable] attribute on them and a serious warning in the comments about not modifying these for fear of breaking integration with the VS extension. Is that mechanism still in use to convey configuration information to the Extension? |
|
Does the extension have a good reason for needing to parse content itself? I expect we'll need to do some refactoring to providing configuration in a less-coupled way. |
|
@clrudolphi @Code-Grump Good news, this issue has been eliminated once I ported SpecFlow to Reqnroll, so these restrictions are not applied anymore. History: In the SpecFlow times, the VS extension loaded the bindings by invoking several SpecFlow classes and with an overridden implementations of different services, particularly the one that loads the configuration (because the configuration in that case was just available as a string and not necessarily in a file at a position where SpecFlow loaded it). This required that the VS extension loads a couple of SpecFlow types, but if these types are changed the integration broke. (Note that one version of VS extension has to work with many versions of SpecFlow/Reqnroll, so changing these interfaces were really a pain.) Now with Reqnroll, I introduced a single static method: BindingProviderService.DiscoverBindings that communicates only with common types (string and Assembly), so the VS extension does not need to load Reqnroll types and therefore the cross-compatibility is much simpler. (The string it returns contains a JSON serialized data structure that cannot be changed, but this structure is special and only used for this interface. See https://github.com/reqnroll/Reqnroll/tree/main/Reqnroll/Bindings/Provider/Data) So the |
Eliminated [Serializable] attributes in Generator classes as no longer needed.
|
Working on incorporating the use of the MS Extensions Configuration package in to the Generator. What will need to be done to the nuspec so that the Microsoft.Extensions.Configuration.* assemblies get loaded properly? At this point, the RuntimeTests and GeneratorTests can be executed successfully. However, the SystemTests fail at the point that the generated solution is compiled ("Could not find file '\DefaultTestProject\DefaultTestProject.csproj'). The folder for the generated project is empty. And the simplest Spec project fails with an error that the GenerateFeatureFileCodeBehindTask failed b/c the 'Microsoft.Extensions.Configuration.Json' assembly could not be found. I've pushed my latest updates to this branch if you'd like to take a look. |
…t}.json file content; where Environment is the value of the DOTNET_Environment environment variable. Modified the ConfigurationLoader such that both code paths that load json-based config use the MSE loader.
|
Added support for Environment-based overrides. |
This is a highly undiscovered area as we never had a framework-related dependency so far in the generator that was not included in the framework by default. In order to be able to load the assembly, somehow you need to ensure that it is included in the For the For the With this, the build will technically work, but then you need to test it with projects that have different versions of the Microsoft.Extensions.Configuration. But since during generation the user's assemblies are not loaded, there should be no conflict. But we need to test this. |
|
@clrudolphi I did a quick review, but there is another issue that we need to clarify (cc @Code-Grump). There are 5 (non-distinct!) groups of configuration data we have right now.
For group 1 (runtime) and group 2 (user config) the use of MS config is a quick win, because
The other groups are more problematic, especially from the dynamic configuration nature's point of view.
Currently Reqnroll works in a way, that for group 3, 4, 5 it allows to delegate the discovery of the JSON config content to the caller and provides an overload where you can pass in the JSON config content as a string. This is used quite heavily:
I have seen that in the PR (here) the content-based loading of configuration has been disabled. Maybe this is temporary only, but I need to highlight that this is potentially breaks the usage of groups 3,4,5 including the VS extension. So we will need to make this working somehow. My suggestion would be the following: We should not allow dynamic config overrides for config settings in group 3,4,5. This is anyway only the minority of the config settings and it is anyway does not make sense to make these dynamic, so this would not limit the possibilities significantly, but would make the generator and the VS extension (and their ongoing development) much easier. Basically we would need to highlight in the docs that these settings cannot be overridden but always read from What do you think? |
Thank you for the clarifications. This helps untangle several points of confusion for me. I will revert the change that dropped the ability to load the config as text content. My intent was to consolidate and simplify the codebase by eventually ripping that stuff out. Now I better understand why it is there. I see three options for moving forward and would like input on which path to use. In considering these, I would propose that our set-up of the use of the configurationManager.AddEnvironmentVariables("DOTNET_");
string envName = configurationManager["Environment"];
_jsonConfigurationFilePath = reqnrollJsonLocator.GetReqnrollJsonFilePath();
if (_jsonConfigurationFilePath != null)
{
var pathWithoutExt = Path.Combine(Path.GetDirectoryName(_jsonConfigurationFilePath),Path.GetFileNameWithoutExtension(_jsonConfigurationFilePath));
var envOverridePath = $"{pathWithoutExt}.{envName}.json";
configurationManager.AddJsonFile(_jsonConfigurationFilePath, optional: true, reloadOnChange: false);
configurationManager.AddJsonFile(envOverridePath, optional: true, reloadOnChange: false);
}
configurationManager.AddEnvironmentVariables(prefix: "REQNROLL__");In the above, the variable The three options:
Option 1 is the simplest to implement as we initialize the If up to me, I would choose option 1. Its incremental and relatively clean. Doesn't preclude moving to the other options later. |
|
@gasparnagy - side question - I see in the code that runtime/* supports a sub-element called dependencies which appear to be dependency registrations. This capability is not included in the Documentation. What are these used for? |
My first attempt at this has not helped. The nuspec for The same two behaviors are occurring. The first is an error reported when building Specs: I've also experimented with removing the 'exclude' line completely from the nuspec for the MsBuild generator and get the same result. I have confirmed by looking at the nuget package via the nuget package explorer that the first-level dependencies are there in the /tasks folder of the package. The second is that the SystemTest generator is failing when generating a project file. The test error output includes: At this point, I'm presuming that solving the first problem also solves the second. But let me know if you feel otherwise. |
|
@clrudolphi could you please open the generated nupkg file as zip and confirm that the necessary dll-s are in the tools folder? If not maybe they were not even in the bin/debug... |
|
@clrudolphi also check if the "building the Specs" problem is from Visual Studio build (the tools/net461 folder is used) or from dotbet build (the tools/netstandard2.0 folder is used). |
In the zip, there is no "tools" folder, but there is a "tasks" folder and all the Microsoft.Extensions.Configuration.* assemblies are in that folder.
Fails when building from the command-line as well. |
|
I have some challenges with the cognitive model we have here. First of all, it seems clear from gasparnagy's explanation that we're currently mixing a lot of configuration concepts into a single file with reqnroll.json. This is made really clear now we're talking about using environment-specific configuration values, where environment means something only during runtime. I think there's an expectation that if you have a configuration file, all the values inside it apply equally - having some values only apply during certain circumstances is non-intuitive behaviour that will trip somebody up eventually. Even if we document "these values cannot be overidden" somebody will first have to try, discover it doesn't work, then read the docs, then raise an issue. Although I don't have any usage information to back this up I think there is a desire for this kind of dynamic configuration. The two default environment values used in ASP.NET projects are "Development" and "Production"; the former is set on machines with the ASP.NET SDK installed, the latter explicitly on production servers. It's often used to enable behaviours that only make sense during design-time and aren't valid otherwise. So I think we should either support the same hierarchical configuration in all contexts, or split up our configuration into distinct design-time and runtime blocks, but mixing configuration scopes within a single file feels wrong. |
to load the reqroll configuration json file and allow for Environment Variable Overrides.
🤔 What's changed?
Modified the configuration loading to leverage the Microsoft.Extensions.Configuration system to load the reqnroll.json file.
⚡️ What's your motivation?
🏷️ What kind of change is this?
♻️ Anything particular you want feedback on?
This PR just demonstrates the feasibility. Other areas to confirm:
📋 Checklist: