rusackas commented on PR #41777: URL: https://github.com/apache/superset/pull/41777#issuecomment-4910072945
Thanks @lucargir, this is a lot of careful work and the deprecation path is a nice touch. The thing I want to be sure of before we merge is the legacy-value precedence. Since `database.port`/`driver` and `cache.port`/`cacheDb`/`celeryDb` ship non-null defaults in values.yaml, Helm's deep-merge fills them in, so the `if`/`coalesce` resolvers never fall through to a user's old `supersetNode.connections.*`. An existing 0.19 install that upgrades without migrating would quietly point at postgres:5432 / redis db 1 even if they'd set mysql:3306 or a different redis db. The unit tests set the legacy port to the same value as the default, so they don't catch it. Defaulting those new keys to `~` and putting the hard default in the coalesce tail would keep the legacy values winning. @villebro, mind sanity-checking the back-compat resolver semantics here? You'd have better instincts than me on what existing deployments lean on. Separately, the nested-map `config.*` path renders `true`/`false`/`null` via `toJson`, which aren't valid in `superset_config.py`, so a dict setting with a bool would break the render... worth guarding or documenting. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
