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

Experiment to use Microsoft.Extensions.Configuration #306

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

clrudolphi
Copy link
Contributor

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?

  • Allow for Environment Variable overrides of configuration settings
  • Allow an extension point such that Plugins and other extensions of Reqnroll can add their own sections to the configuration file.

🏷️ What kind of change is this?

  • 🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)
  • ⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

This PR just demonstrates the feasibility. Other areas to confirm:

  • is this the best way to go about this? Perhaps there is another seam to use in place of squirming in between the Configuration Loader and the JsonConfigurationLoader?
  • Impacts to Generation? (This only demonstrates loading at runtime)
  • Determine whether this approach could be used to load the old appSettings.config xml

📋 Checklist:

  • [X ] I've changed the behaviour of the code
    • [ X] I have added/updated tests to cover my changes.
  • [ X] My change requires a change to the documentation.
    • I have updated the documentation accordingly.
  • [X ] Users should know about my change
    • I have added an entry to the "[vNext]" section of the CHANGELOG, linking to this pull request & included my GitHub handle to the release contributors list.

…roll configuration json file and allow for Environment Variable Overrides.
@Code-Grump
Copy link
Contributor

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.

@clrudolphi
Copy link
Contributor Author

@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?
These data structures seem to rely on the contents of the configuration files being read in as text (eg, the ReqnrollConfigurationHolder) and then parsed later after reading. This approach is going to get in the way of using the MS.Extension.Configuration system.

@Code-Grump
Copy link
Contributor

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.

@gasparnagy
Copy link
Contributor

@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 [Serializable] attributes can be ignored/removed and you can now also change the classes that have the warning (and/or remove the warning).

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