Skip to content

Commit

Permalink
Better config management (#2044)
Browse files Browse the repository at this point in the history
* Better config loader logs
* It's possible to always load reference role configs by setting `"distage.roles.always-include-reference-role-configs"` boolean in the boostrap role app context. 
* Tests do not attempt to load per-package reference configs anymore
  • Loading branch information
pshirshov authored Dec 6, 2023
1 parent d4ca060 commit c1f1996
Show file tree
Hide file tree
Showing 19 changed files with 111 additions and 65 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -58,19 +58,11 @@ object ConfigLoadResult {
sealed trait ConfigSource

object ConfigSource {
final case class Resource(name: String, kind: ResourceConfigKind) extends ConfigSource {
final case class Resource(name: String) extends ConfigSource {
override def toString: String = s"resource:$name"
}

final case class File(file: java.io.File) extends ConfigSource {
override def toString: String = s"file:$file"
}
}

sealed trait ResourceConfigKind

object ResourceConfigKind {
case object Primary extends ResourceConfigKind

case object Development extends ResourceConfigKind
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
somesection {
somevalue = 1
}
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package izumi.distage.framework.services

import distage.config.AppConfig

trait ConfigLoader extends AbstractConfigLoader

object ConfigLoader {
def empty: ConfigLoader = () => ???
def empty: ConfigLoader = (_: String) => AppConfig.empty
}
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,7 @@ abstract class RoleCheckableApp[F[_]](override implicit val tagK: TagK[F]) exten
}

private[this] def specificResourceConfigLoader(classLoader: ClassLoader, resourceName: String): ConfigLoader = {
() =>
(_: String) =>
val cfg = ConfigFactory.parseResources(classLoader, resourceName).resolve()
if (cfg.origin().resource() eq null) {
throw new DIConfigReadException(s"Couldn't find a config resource with name `$resourceName` - file not found", null)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -262,7 +262,7 @@ object PlanCheck {
val reachableKeys = providedKeys ++ planVerifierResult.visitedKeys

val configIssues = if (checkConfig) {
val realAppConfig = configLoader.loadConfig()
val realAppConfig = configLoader.loadConfig("compile-time validation")
reportEffectiveConfig(realAppConfig.config.origin().toString)

module.iterator
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,14 +22,16 @@ final case class PlanCheckInput[F[_]](
bsPlugins: LoadedPlugins,
)
object PlanCheckInput {
private val emptyConfigArgs = ConfigArgsProvider.const(ConfigLoader.Args(None, List.empty, alwaysIncludeReferenceRoleConfigs = true))

def apply[F[_]](
module: ModuleBase,
roots: Roots,
roleNames: Set[String] = Set.empty,
configLoader: ConfigLoader = {
val logger = IzLogger()
val merger = new ConfigMergerImpl(logger)
new ConfigLoader.LocalFSImpl(logger, merger, ConfigLocationProvider.Default, ConfigArgsProvider.Empty)
new ConfigLoader.LocalFSImpl(logger, merger, ConfigLocationProvider.Default, emptyConfigArgs)
},
appPlugins: LoadedPlugins = LoadedPlugins.empty,
bsPlugins: LoadedPlugins = LoadedPlugins.empty,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package izumi.distage.framework.services

import izumi.distage.config.model.{GenericConfigSource, RoleConfig}
import izumi.distage.model.definition.Id
import izumi.distage.roles.RoleAppMain
import izumi.distage.roles.model.meta.RolesInfo
import izumi.fundamentals.platform.cli.model.raw.RawAppArgs
Expand All @@ -14,18 +15,16 @@ trait ConfigArgsProvider {

object ConfigArgsProvider {
def const(args: ConfigLoader.Args): ConfigArgsProvider = new ConfigArgsProvider.Const(args)
def empty: ConfigArgsProvider = ConfigArgsProvider.Empty

open class Const(args0: ConfigLoader.Args) extends ConfigArgsProvider {
override def args(): ConfigLoader.Args = args0
}

object Empty extends ConfigArgsProvider.Const(ConfigLoader.Args(None, List.empty))

@nowarn("msg=Unused import")
class Default(
parameters: RawAppArgs,
rolesInfo: RolesInfo,
alwaysIncludeReferenceRoleConfigs: Boolean @Id("distage.roles.always-include-reference-role-configs"),
) extends ConfigArgsProvider {
override def args(): ConfigLoader.Args = {
import scala.collection.compat.*
Expand All @@ -46,7 +45,7 @@ object ConfigArgsProvider {
}
val maybeGlobalConfig = parameters.globalParameters.findValue(RoleAppMain.Options.configParam).asFile

ConfigLoader.Args(maybeGlobalConfig, roleConfigs)
ConfigLoader.Args(maybeGlobalConfig, roleConfigs, alwaysIncludeReferenceRoleConfigs)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,14 @@ trait ConfigLoader extends AbstractConfigLoader

@nowarn("msg=Unused import")
object ConfigLoader {
def empty: ConfigLoader = () => AppConfig(ConfigFactory.empty(), List.empty, List.empty)
def empty: ConfigLoader = (_: String) => AppConfig(ConfigFactory.empty(), List.empty, List.empty)

import scala.collection.compat.*

final case class Args(
global: Option[File],
configs: List[RoleConfig],
alwaysIncludeReferenceRoleConfigs: Boolean,
)
final class ConfigLoaderException(message: String, val failures: List[Throwable]) extends DIException(message)

Expand All @@ -69,16 +70,22 @@ object ConfigLoader {
) extends ConfigLoader {
protected def resourceClassLoader: ClassLoader = getClass.getClassLoader

def loadConfig(): AppConfig = {
def loadConfig(clue: String): AppConfig = {
val configArgs = args.args()

val maybeLoadedRoleConfigs = configArgs.configs.map {
rc =>
val defaults = configLocation.forRole(rc.role).map(loadConfigSource)
val loaded = rc.configSource match {
case GenericConfigSource.ConfigFile(file) =>
Seq(loadConfigSource(ConfigSource.File(file)))
val provided = Seq(loadConfigSource(ConfigSource.File(file)))
if (configArgs.alwaysIncludeReferenceRoleConfigs) {
provided ++ defaults
} else {
provided
}
case GenericConfigSource.ConfigDefault =>
configLocation.forRole(rc.role).map(loadConfigSource)
defaults
}
(rc, loaded)
}
Expand Down Expand Up @@ -110,7 +117,7 @@ object ConfigLoader {
logger.error(s"Cannot load configuration: ${failures.toList.niceList() -> "failures"}")
throw new ConfigLoaderException(s"Cannot load configuration: ${failures.toList.niceList()}", value.map(_.failure).toList)
case Right((shared, role)) =>
val merged = merger.addSystemProps(merger.merge(shared, role))
val merged = merger.addSystemProps(merger.merge(shared, role, clue))
AppConfig(merged, shared, role)
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package izumi.distage.framework.services

import izumi.distage.config.model.{ConfigSource, ResourceConfigKind}
import izumi.distage.config.model.ConfigSource

trait ConfigLocationProvider {
def forRole(roleName: String): Seq[ConfigSource]
Expand All @@ -23,9 +23,9 @@ object ConfigLocationProvider {

private def defaultConfigReferences(name: String): Seq[ConfigSource] = {
Seq(
ConfigSource.Resource(s"$name.conf", ResourceConfigKind.Primary),
ConfigSource.Resource(s"$name-reference.conf", ResourceConfigKind.Primary),
ConfigSource.Resource(s"$name-reference-dev.conf", ResourceConfigKind.Development),
ConfigSource.Resource(s"$name.conf"),
ConfigSource.Resource(s"$name-reference.conf"),
ConfigSource.Resource(s"$name-reference-dev.conf"),
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,45 +10,64 @@ import scala.jdk.CollectionConverters.*
import scala.util.Try

trait ConfigMerger {
def merge(shared: List[ConfigLoadResult.Success], role: List[LoadedRoleConfigs]): Config
def mergeFilter(shared: List[ConfigLoadResult.Success], role: List[LoadedRoleConfigs], filter: LoadedRoleConfigs => Boolean): Config
def merge(shared: List[ConfigLoadResult.Success], role: List[LoadedRoleConfigs], clue: String): Config
def mergeFilter(shared: List[ConfigLoadResult.Success], role: List[LoadedRoleConfigs], filter: LoadedRoleConfigs => Boolean, clue: String): Config
def foldConfigs(roleConfigs: List[ConfigLoadResult.Success]): Config
def addSystemProps(config: Config): Config
}

object ConfigMerger {
class ConfigMergerImpl(logger: IzLogger @Id("early")) extends ConfigMerger {
override def merge(shared: List[ConfigLoadResult.Success], role: List[LoadedRoleConfigs]): Config = {
mergeFilter(shared, role, _.roleConfig.active)
override def merge(shared: List[ConfigLoadResult.Success], role: List[LoadedRoleConfigs], clue: String): Config = {
mergeFilter(shared, role, _.roleConfig.active, clue)
}

override def mergeFilter(shared: List[ConfigLoadResult.Success], role: List[LoadedRoleConfigs], filter: LoadedRoleConfigs => Boolean): Config = {
val cfgInfo = (shared ++ role.flatMap(_.loaded)).map(c => c.clue)
logger.info(s"Using system properties with fallback ${cfgInfo.niceList() -> "config files"}")
override def mergeFilter(shared: List[ConfigLoadResult.Success], role: List[LoadedRoleConfigs], filter: LoadedRoleConfigs => Boolean, clue: String): Config = {
val nonEmptyShared = shared.filterNot(_.config.isEmpty)
val roleConfigs = role.flatMap(_.loaded)
val nonEmptyRole = roleConfigs.filterNot(_.config.isEmpty)

val toMerge = shared ++ role.filter(filter).flatMap(_.loaded)
val toMerge = (shared ++ role.filter(filter).flatMap(_.loaded)).filterNot(_.config.isEmpty)

foldConfigs(toMerge)
val folded = foldConfigs(toMerge)

val repr = toMerge.map(c => c.clue)

val sub = logger("config context" -> clue)
sub.info(s"Config input: ${shared.size -> "shared configs"} of which ${nonEmptyShared.size -> "non empty shared configs"}")
sub.info(s"Config input: ${roleConfigs.size -> "role configs"} of which ${nonEmptyRole.size -> "non empty role configs"}")
sub.info(s"Output config has ${folded.entrySet().size() -> "root nodes"}")
sub.info(s"The following configs were used (ascending priority): ${repr.niceList() -> "used configs"}")

val configRepr = (shared.map(c => (c.clue, true)) ++ role.flatMap(r => r.loaded.map(c => (s"${c.clue}, role=${r.roleConfig.role}", filter(r)))))
.map(c => s"${c._1}, active = ${c._2}")
logger.debug(s"Full list of processed configs: ${configRepr.niceList() -> "locations"}")

folded
}

def foldConfigs(roleConfigs: List[ConfigLoadResult.Success]): Config = {
roleConfigs.reverse // rightmost config will have the highest priority
.foldLeft(ConfigFactory.empty()) {
case (acc, loaded) =>
verifyConfigs(loaded, acc)
acc.withFallback(loaded.config)
}
val fallbackOrdered = roleConfigs.reverse // rightmost config has the highest priority, so we need it to become leftmost

verifyConfigs(fallbackOrdered)

fallbackOrdered.foldLeft(ConfigFactory.empty()) {
case (acc, loaded) =>
acc.withFallback(loaded.config)
}
}

protected def verifyConfigs(loaded: ConfigLoadResult.Success, acc: Config): Unit = {
val duplicateKeys = getKeys(acc) intersect getKeys(loaded.config)
if (duplicateKeys.nonEmpty) {
loaded.src match {
case ConfigSource.Resource(_, ResourceConfigKind.Development) =>
logger.debug(s"Some keys in supplied ${loaded.src -> "development config"} duplicate already defined keys: ${duplicateKeys.niceList() -> "keys" -> null}")
case _ =>
logger.warn(s"Some keys in supplied ${loaded.src -> "config"} duplicate already defined keys: ${duplicateKeys.niceList() -> "keys" -> null}")
}
private def verifyConfigs(fallbackOrdered: List[ConfigLoadResult.Success]): Unit = {
import izumi.fundamentals.collections.IzCollections.*
val keyIndex = fallbackOrdered
.filter(_.src.isInstanceOf[ConfigSource.Resource])
.flatMap(c => getKeys(c.config).map(key => (key, c)))

keyIndex.toUniqueMap(identity) match {
case Left(value) =>
val diag = value.map { case (key, configs) => s"$key is defined in ${configs.map(_.clue).niceList(prefix = "* ").shift(2)}" }
logger.warn(s"Reference config resources have ${diag.niceList() -> "conflicting keys"}")
case Right(_) =>
}
}

Expand All @@ -66,10 +85,15 @@ object ConfigMerger {
}

override def addSystemProps(config: Config): Config = {
ConfigFactory
val result = ConfigFactory
.systemProperties()
.withFallback(config)
.resolve()
logger.info(
s"Config with ${config.entrySet().size() -> "root nodes"} has been enhanced with system properties, new config has ${result.entrySet().size() -> "new root nodes"}"
)

result
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ class RoleAppBootConfigModule[F[_]: TagK: DefaultModule]() extends ModuleDef {
make[ConfigLoader].from[ConfigLoader.LocalFSImpl]
make[ConfigMerger].from[ConfigMerger.ConfigMergerImpl]
make[ConfigLocationProvider].from(ConfigLocationProvider.Default)
// make[ConfigLoader.Args].from(ConfigLoader.Args.makeConfigLoaderArgs _)
make[ConfigArgsProvider].from[ConfigArgsProvider.Default]
make[Boolean].named("distage.roles.always-include-reference-role-configs").fromValue(false)
make[AppConfig].from {
(configLoader: ConfigLoader) =>
configLoader.loadConfig()
configLoader.loadConfig("application startup")
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ final class ConfigWriter[F[_]](
val loaded = index(roleId)

// TODO: mergeFilter considers system properties, we might want to AVOID that in configwriter
val mergedRoleConfig = configMerger.mergeFilter(appConfig.shared, List(loaded), _ => true)
val mergedRoleConfig = configMerger.mergeFilter(appConfig.shared, List(loaded), _ => true, "configwriter")
writeConfig(options, fileNameFull, mergedRoleConfig, subLogger)

minimizedConfig(mergedRoleConfig, role)
Expand Down Expand Up @@ -108,7 +108,7 @@ final class ConfigWriter[F[_]](

// TODO: mergeFilter considers system properties, we might want to AVOID that in configwriter
// TODO: here we accept all the role configs regardless of them being active or not, that might resolve cross-role conflicts in unpredictable manner
val fullConfig = configMerger.mergeFilter(appConfig.shared, roleConfigs, _ => true)
val fullConfig = configMerger.mergeFilter(appConfig.shared, roleConfigs, _ => true, "configwriter")
val correctedAppConfig = appConfig.copy(config = fullConfig, roles = roleConfigs)

val bootstrapOverride = new BootstrapModuleDef {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
somekey = 1
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package com.github.pshirshov.test.plugins

import com.github.pshirshov.test.plugins.StaticTestMain.staticTestMainPlugin
import distage.{ClassConstructor, TagK}
import izumi.functional.quasi.QuasiApplicative
import distage.{ClassConstructor, ModuleDef, TagK}
import izumi.distage.model.definition.Module
import izumi.distage.modules.DefaultModule2
import izumi.distage.plugins.{PluginConfig, PluginDef}
import izumi.distage.roles.RoleAppMain
import izumi.distage.roles.RoleAppMain.ArgV
import izumi.distage.roles.model.definition.RoleModuleDef
import izumi.functional.bio.Async2
import izumi.functional.quasi.QuasiApplicative
import izumi.fundamentals.platform.functional.Identity
import izumi.reflect.TagKK
import logstage.LogIO2
Expand All @@ -29,6 +31,11 @@ object StaticTestMainBadEffect extends RoleAppMain.LauncherIdentity {
}

class StaticTestMainLogIO2[F[+_, +_]: TagKK: Async2: DefaultModule2] extends RoleAppMain.LauncherBIO[F] {

override protected def roleAppBootOverrides(argv: ArgV): Module = super.roleAppBootOverrides(argv) ++ new ModuleDef {
make[Boolean].named("distage.roles.always-include-reference-role-configs").fromValue(true)
}

override protected def pluginConfig: PluginConfig =
PluginConfig
.cached("com.github.pshirshov.test.plugins")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -400,9 +400,10 @@ class RoleAppTest extends AnyWordSpec with WithProperties {
DebugProperties.`izumi.distage.roles.activation.ignore-unknown`.name -> "true",
DebugProperties.`izumi.distage.roles.activation.warn-unset`.name -> "false",
) {
val checkTestGoodResouce = getClass.getResource("/check-test-good.conf").getPath
new StaticTestMainLogIO2[zio.IO].main(Array("-ll", logLevel, "-c", checkTestGoodResouce, ":" + StaticTestRole.id))
// new StaticTestMainLogIO2[monix.bio.IO].main(Array("-ll", logLevel, "-c", checkTestGoodResouce, ":" + StaticTestRole.id))
val checkTestGoodRes = getClass.getResource("/check-test-good.conf").getPath
val customRoleConfigRes = getClass.getResource("/custom-role.conf").getPath
new StaticTestMainLogIO2[zio.IO].main(Array("-ll", logLevel, "-c", checkTestGoodRes, ":" + StaticTestRole.id, "-c", customRoleConfigRes))
// new StaticTestMainLogIO2[monix.bio.IO].main(Array("-ll", logLevel, "-c", checkTestGoodRes, ":" + StaticTestRole.id))
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package izumi.distage.framework.services
import izumi.distage.config.model.AppConfig

trait AbstractConfigLoader {
def loadConfig(): AppConfig
def loadConfig(clue: String): AppConfig

final def map(f: AppConfig => AppConfig): ConfigLoader = () => f(loadConfig())
final def map(f: AppConfig => AppConfig): ConfigLoader = (clue: String) => f(loadConfig(clue))
}
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ final case class TestConfig(
parallelSuites: Parallelism = Parallelism.Unlimited,
parallelTests: Parallelism = Parallelism.Unlimited,
// other options
configBaseName: String,
configBaseName: String = "test",
configOverrides: Option[AppConfig] = None,
bootstrapFactory: BootstrapFactory = BootstrapFactory.Impl,
planningOptions: PlanningOptions = PlanningOptions(),
Expand All @@ -96,8 +96,10 @@ final case class TestConfig(

}
object TestConfig {
@deprecated("Use TestConfig() constructor instead, always provide pluginConfig explicitly", "1.2.3")
def forSuite(clazz: Class[?]): TestConfig = {
val packageName = clazz.getPackage.getName

TestConfig(
pluginConfig = PluginConfig.cached(Seq(packageName)),
configBaseName = s"${packageName.split('.').last}-test",
Expand Down
Loading

0 comments on commit c1f1996

Please sign in to comment.