Allow integrating data into IConfigManager by andreniggemann · Pull Request #1259 · PHOENIXCONTACT/MORYX-Framework

andreniggemann · 2026-06-09T14:09:58Z

Summary

Allow populating ModuleConfigs from IConfiguration entries.
This is our way to allow injecting secrets and configuration from ConfigurationProviders based on Microsoft.Extensions.Configuration into the MORYX IConfigManager.

To properly handle nested configs (an important part of the IConfiguration concept the ValueProviderExecutor has been extended to allow access to context information through the new IContextAwareValueProvider interface.

Linked Issues

CLoses #710
It does not depend on the Password Attribute as mentionend in the Issue, because the Password Attribute still means that the value will be serialized to disk and over the API.

Relevant logs and/or screenshots

Breaking Changes

Checklist for Submitter

I have tested these changes locally
I have updated documentation as needed
I have added or updated tests as appropriate
I have used clear and descriptive commit messages

Review

Typical tasks

Merge request is well described
Added Obsolete attributes if necessary
Critical sections are documented in code
Tests available or extended
Documentation is created / updated
Running in test environment
Manual is created / updated
Ports to other maintained versions are created

Clean code

All unused references are removed
Clean code rules are respected with passion (naming, ...)
Avoid copy and pasted code snippets

1nf0rmagician

I'm happy with the concept and think it is a nicely limited change!

I already went quite into the details with the review, but please take this not as a being overly picky with the PR. I needed to read it closely for full understanding and when I spotted something I didn't want to forget about it later when you had brought it out of the draft state 😬🤗

1nf0rmagician · 2026-06-09T21:20:07Z

+    private static string KeyFromAttribute(object config, InjectedConfigAttribute attribute)
+    {
+        var configKey = attribute.ConfigKey;
+        if (attribute.FromProperty)


You could throw nice exceptions if someone missconfigured the property name or the property is not a string property. I think currently the behaviour is rather undefined

1nf0rmagician · 2026-06-09T21:22:34Z

+        return configKey;
+    }
+
+    private object GetRootConfigKey(Stack<ExecutorLevel> levels)


This should return string shouldn'T it?

1nf0rmagician · 2026-06-09T21:26:38Z

+        return ValueProviderResult.Skipped;
+    }
+
+    private string GetConfigKeyNameCached(Stack<ExecutorLevel> levels)


I must admit I do not quite get the caching part in this, maybe a little explanation comment/example comment would help me🙈

It only stores the config key for an element it has already seen in the dict attached to the stackframe.
This means that if we have nested configuration, you don't have to build the entire path from scratch each time.

1nf0rmagician · 2026-06-09T21:33:11Z

While you have implemented collection handling (as far as i can see) there is no test for this

dbeuchler

I like that implementation! Thanks for that. (just small things from my end: code format, braces, no one liners...).

One thing to think about: Why do you have touched the default ConfigManager? I think this can easily archived by derive from it and provide an additional implementation which the application developer can select if required. Keeps the default implementation as it is and for extended usage, it is possible to use it in application.

Before we merge it to dev and make an release instantly, could we apply this into an application (I don't mean the demo) to test if it behaves as wanted?

andreniggemann · 2026-06-10T07:30:58Z

I like that implementation! Thanks for that. (just small things from my end: code format, braces, no one liners...).

Thank you very much :)

One thing to think about: Why do you have touched the default ConfigManager? I think this can easily archived by derive from it and provide an additional implementation which the application developer can select if required. Keeps the default implementation as it is and for extended usage, it is possible to use it in application.

That's good idea. I would add a switch to the AddMoryxKernel Extension method that allows opting in to the new behavior, if that's ok with you

Before we merge it to dev and make an release instantly, could we apply this into an application (I don't mean the demo) to test if it behaves as wanted?

That's my plan. That's why it's still in draft. Additionally I intend to add a regression test that takes the ModuleConfigs of the core Modules and checks that, if the IConfiguration is empty, you get the exact same configs.

dbeuchler · 2026-06-10T08:46:44Z

That's good idea. I would add a switch to the AddMoryxKernel Extension method that allows opting in to the new behavior, if that's ok with you

Instead of adding options to the extensions, another idea: You can replace registered services with services.Replace? Maybe a simple hint in the docs and this behavior is fully optional.

andreniggemann · 2026-06-10T09:00:28Z

Isn't it completely optional if I define the Method as AddMoryxKernel(bool configurationIntegration = false)?

Nobody would have to change their code, it's much more discoverable as a hint in the docs and the "new ConfigManager" (not sure on naming yet, or if even needs a new type. Maybe it could just be a different constructor that takes a list of IValueProviders) can be internal.

By the way: Is there a reason for the existing ConfigManager being public? In an application we always use it through the IConfigManager interface

1nf0rmagician · 2026-06-12T13:18:45Z

+This can be set in appsettings.json like this:
+
+
+You can archive this by adding the following to your appsettings.json


little duplicate

1nf0rmagician · 2026-06-12T13:30:42Z

+        return manager;
+    }
+
+    // TODO: Consider removing -> Tested by MicrsoftExtensionConfigProviderTests


Open todos in this file, if you ask me, I like keeping some integration tests (if they actually test just a littel bit more than the MicrsoftExtensionConfigProviderTests.

Ok, than I'll remove the TODOs. They test the integrate a little bit

to allow injecting secrets and arbitrary config into the Moryx Configuration System Refactor to ValueProvider and add (failing) test to ensure the secret is not written out to file Rewrite to allow injecting arbitrary config properties from IConfiguration. * Add IContextAwareConfigProvider to keep track of nesting * Implement the new interface in MsExtensionsConfigProvider * Add Tests * Rename MsExtensionsConfigProvider to MicrosoftExtensionConfigProvider and rename test accordingly * Rename InjectedConfigAttribute to ProvidedConfigAttribute * Move tests for integration of the new ConfigProvider into the ConfigManager into ConfigManagerWithConfigProviderTests * Role back changes to ConfigManager so that it behaves as previously by default and add parameter to AddMoryxKernelExtension Method to allow opting in to the new behavior * Add tests for populating Lists using the new Provider and fix the implementation for that case. * Add error logs when we can't bind values for when something goes wrong

andreniggemann requested a review from 1nf0rmagician June 9, 2026 14:09

1nf0rmagician assigned andreniggemann Jun 9, 2026

1nf0rmagician added the enhancement New feature or request label Jun 9, 2026

1nf0rmagician added this to the Framework 10.x milestone Jun 9, 2026

1nf0rmagician reviewed Jun 9, 2026

View reviewed changes

dbeuchler reviewed Jun 10, 2026

View reviewed changes

andreniggemann marked this pull request as ready for review June 12, 2026 08:55

andreniggemann requested review from 1nf0rmagician and dbeuchler June 12, 2026 09:32

1nf0rmagician previously approved these changes Jun 12, 2026

View reviewed changes

andreniggemann dismissed 1nf0rmagician’s stale review via 5461842 June 12, 2026 13:46

dbeuchler reviewed Jun 15, 2026

View reviewed changes

Comment thread src/Moryx.Runtime.Kernel/KernelServiceCollectionExtensions.cs

dbeuchler approved these changes Jun 18, 2026

View reviewed changes

andreniggemann added 3 commits June 18, 2026 12:57

Improve documentation

c66cd57

Add note to remove SharedValueProvider on future branch

a2096cc

andreniggemann force-pushed the feature/configuration-providers branch from a7100a0 to a2096cc Compare June 18, 2026 10:58

andreniggemann merged commit 8397774 into dev Jun 18, 2026
7 checks passed

andreniggemann deleted the feature/configuration-providers branch June 18, 2026 11:23

andreniggemann mentioned this pull request Jun 18, 2026

Allow optional integration of configuration providers #894

Closed

		This can be set in appsettings.json like this:


		You can archive this by adding the following to your appsettings.json

Conversation

andreniggemann commented Jun 9, 2026 • edited by dbeuchler Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Linked Issues

Relevant logs and/or screenshots

Breaking Changes

Checklist for Submitter

Review

Uh oh!

1nf0rmagician left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dbeuchler left a comment

Choose a reason for hiding this comment

Uh oh!

andreniggemann commented Jun 10, 2026 • edited by dbeuchler Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dbeuchler commented Jun 10, 2026

Uh oh!

andreniggemann commented Jun 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andreniggemann commented Jun 9, 2026 •

edited by dbeuchler

Loading

andreniggemann commented Jun 10, 2026 •

edited by dbeuchler

Loading