Allow optional integration of configuration providers by jsonBackup · Pull Request #894 · PHOENIXCONTACT/MORYX-Framework

jsonBackup · 2025-12-03T12:58:27Z

1nf0rmagician

First draft looks promising to me 👍 The following point you could consider next:

Add/Modify documentation accordingly
Add a short tutorial on how to populate config values e.g. in the MQTT or OPC UA Driver config from environment variables
Add code or a seperate ticket for nested properties in objects in the config
Clean up some of the code, test names, comments, etc.

I think the limitation to string properties could only be overcome with the implicit config population and since you only consider password properties there they will need to be of type string to 😅 But I think you could split this up into a separate issue, if you already have an idea on how to solve this, keeping this change limited to string properties would be alright for me.

1nf0rmagician · 2025-12-07T20:01:53Z

src/Moryx.Resources.Management/ModuleController/ModuleController.cs

        public ModuleController(IModuleContainerFactory containerFactory, IConfigManager configManager, ILoggerFactory loggerFactory, IDbContextManager contextManager)
            : base(containerFactory, configManager, loggerFactory)
        {
+            // first tests:


Nice that this works, but I think it should not be the focus of this pr 🙂

1nf0rmagician · 2025-12-07T20:02:25Z

src/Moryx.Resources.Management/ModuleController/MoviesConfig.cs

Remember to remove before merge

1nf0rmagician · 2025-12-07T20:03:11Z

src/Moryx.Runtime.Kernel/KernelServiceCollectionExtensions.cs

+
+            // NEW: hook up IConfiguration from the host
+            var configuration = serviceProvider.GetService<IConfiguration>();
+            if (configuration != null)


As the property is nullable no need for the extra check, is it?

isn't the property filled by the service collection?!

1nf0rmagician · 2025-12-07T20:07:24Z

src/Moryx.Runtime.Kernel/Configuration/ConfigManager.cs

                config = CreateConfig(confType, ConfigState.Generated, "Config file not found! Running on default values.");
+
+                // Defaults & shared configs
+                ValueProviderExecutor.Execute(config, new ValueProviderExecutorSettings().AddProviders(ValueProviders));


What does this line do?

Executes the value provider (e.g- default values...) I wonder why this was not applied before?

1nf0rmagician · 2025-12-07T20:08:48Z

src/Moryx.Runtime.Kernel/Configuration/ConfigManager.cs

            _sharedProvider = new SharedConfigProvider(this);
        }

+        protected virtual void ApplyConfigurationProviders(object config)


Can't you already limit this to ConfigBase?

1nf0rmagician · 2025-12-07T20:13:03Z

src/Moryx.Runtime.Kernel/Configuration/ConfigManager.cs

+                if (string.IsNullOrWhiteSpace(candidateKey))
+                    continue;
+
+                var providerValue = Configuration[candidateKey!];


What happens here if multiple providers are available?

1nf0rmagician · 2025-12-07T20:14:11Z

src/Moryx.Runtime.Kernel/Configuration/ConfigManager.cs

+                if (!string.IsNullOrEmpty(providerValue))
+                {
+                    // Replace placeholder with the actual secret
+                    prop.SetValue(config, providerValue);


Add logging that a variable was used from a conifguration provider, if available from which configuration provider. But do NOT log the variable value

dbeuchler · 2025-12-08T12:45:55Z

src/Moryx.Runtime.Kernel/Configuration/ConfigManager.cs

+                if (!string.IsNullOrWhiteSpace(currentValue) && currentValue!.Contains(":"))
+                {
+                    // Config file holds the key directly
+                    candidateKey = currentValue;
+                }


This condition is not enough -do we have other indicators?

dbeuchler · 2025-12-08T12:48:54Z

src/Moryx.Runtime.Kernel/Configuration/ConfigManager.cs


                    ValueProviderExecutor.Execute(config, new ValueProviderExecutorSettings().AddProviders(ValueProviders));
+
+                    // NEW: overlay values from Microsoft.Extensions.Configuration providers


Remove this info "NEW"...

dbeuchler · 2025-12-08T12:48:59Z

src/Moryx.Runtime.Kernel/Configuration/ConfigManager.cs

+                // Defaults & shared configs
+                ValueProviderExecutor.Execute(config, new ValueProviderExecutorSettings().AddProviders(ValueProviders));
+
+                // NEW: even here we can try to fill from providers


Remove this info "NEW"...

dbeuchler · 2025-12-08T12:49:37Z

src/Moryx.Runtime.Kernel/Configuration/ConfigManager.cs

                config = CreateConfig(confType, ConfigState.Generated, "Config file not found! Running on default values.");
+
+                // Defaults & shared configs
+                ValueProviderExecutor.Execute(config, new ValueProviderExecutorSettings().AddProviders(ValueProviders));


Executes the value provider (e.g- default values...) I wonder why this was not applied before?

dbeuchler · 2025-12-08T12:50:29Z

src/Moryx.Runtime.Kernel/KernelServiceCollectionExtensions.cs

+
+            // NEW: hook up IConfiguration from the host
+            var configuration = serviceProvider.GetService<IConfiguration>();
+            if (configuration != null)


isn't the property filled by the service collection?!

dbeuchler · 2025-12-08T12:52:25Z

src/Tests/Moryx.Runtime.Kernel.Tests/Configuration/ConfigManagerTests.cs

+        private ConfigManager CreateManager(IConfiguration configuration)
+        {
+            return new ConfigManager
+            {
+                ConfigDirectory = _tempDir,
+                Configuration = configuration
+            };
+        }


Private methods to the bottom of the class -> under the tests

dbeuchler · 2025-12-08T12:53:45Z

src/Moryx.Runtime.Kernel/Configuration/ConfigManager.cs

+        // NEW: hook into Microsoft.Extensions.Configuration
+        public IConfiguration? Configuration { get; set; }


Remove this info "NEW"..., add a well descriptive Summary

wip: Allow optional integration of configuration providers

ae23077

1nf0rmagician requested review from 1nf0rmagician and dbeuchler December 7, 2025 19:58

1nf0rmagician assigned jsonBackup Dec 7, 2025

1nf0rmagician added this to the Framework 8.x milestone Dec 7, 2025

1nf0rmagician added the enhancement New feature or request label Dec 7, 2025

1nf0rmagician reviewed Dec 7, 2025

View reviewed changes

dbeuchler reviewed Dec 8, 2025

View reviewed changes


		ValueProviderExecutor.Execute(config, new ValueProviderExecutorSettings().AddProviders(ValueProviders));

		// NEW: overlay values from Microsoft.Extensions.Configuration providers

		// NEW: hook into Microsoft.Extensions.Configuration
		public IConfiguration? Configuration { get; set; }

Conversation

jsonBackup commented Dec 3, 2025

Uh oh!

1nf0rmagician left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

1nf0rmagician left a comment •

edited

Loading