[GitHub] tysonnorris commented on a change in pull request #3198: move network + dns container args to pureconfig

[GitBox <gi...@apache.org>]

tysonnorris commented on a change in pull request #3198: move network + dns container args to pureconfig
URL: https://github.com/apache/incubator-openwhisk/pull/3198#discussion_r162393505
 
 

 ##########
 File path: common/scala/src/main/scala/whisk/core/containerpool/ContainerFactory.scala
 ##########
 @@ -57,3 +65,23 @@ trait ContainerFactoryProvider extends Spi {
                           instance: InstanceId,
                           parameters: Map[String, Set[String]]): ContainerFactory
 }
+
+object ContainerFactory {
+
+  val containerArgsConfig = {
+    //since typesafe (lightbend) Config does NOT support parsing complex objects from system properties, we need a ConfigReader
+    //see https://github.com/lightbend/config/blob/master/HOCON.md#java-properties-mapping for details on properties handling from typesafe Config
+    //see pureconfig docs at https://pureconfig.github.io/docs/combinators.html
 
 Review comment:
   I tried to be as specific as possible in the comment :)
   
   It is this: _typesafe (lightbend) Config does NOT support parsing complex objects from system properties_ i.e. when you override values via system properties (e.g. the `CONFIG_*` env vars in our case), they are always read as String type, never converted to List/Map/Object/etc.
   
   So, in your example, you did not override the config from system properties.
   
   IMHO this is a real flaw in lightbend/typesafe config, and we just need to be very careful about what types are parsed from the config, and make sure the override via system property works as you expect. I saw some similar logged issues with lightbend config, along with some arguments as to why it won't be fixed...
   
   In fact, I just notice the same flaw exists for the `dns-servers` property - I'll add a config reader for that as well. Now I'm also wonder if the DockerClientTimeoutConfig will exhibit the same issue, if you try to override via system properties.
   
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services