Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(refactor): load configuration and merge recursively #15405

Merged
merged 6 commits into from Jun 28, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion setup.cfg
Expand Up @@ -30,7 +30,7 @@ combine_as_imports = true
include_trailing_comma = true
line_length = 88
known_first_party = superset
known_third_party =alembic,apispec,backoff,bleach,cachelib,celery,click,colorama,contextlib2,cron_descriptor,croniter,cryptography,dateutil,deprecation,flask,flask_appbuilder,flask_babel,flask_caching,flask_compress,flask_jwt_extended,flask_login,flask_migrate,flask_sqlalchemy,flask_talisman,flask_testing,flask_wtf,freezegun,geohash,geopy,graphlib,holidays,humanize,isodate,jinja2,jwt,markdown,markupsafe,marshmallow,marshmallow_enum,msgpack,numpy,pandas,parameterized,parsedatetime,pathlib2,pgsanity,pkg_resources,polyline,prison,pyarrow,pyhive,pyparsing,pytest,pytz,redis,requests,retry,selenium,setuptools,simplejson,slack,sqlalchemy,sqlalchemy_utils,sqlparse,typing_extensions,werkzeug,wtforms,wtforms_json,yaml
known_third_party =alembic,apispec,backoff,bleach,cachelib,celery,click,colorama,contextlib2,cron_descriptor,croniter,cryptography,dateutil,deprecation,flask,flask_appbuilder,flask_babel,flask_caching,flask_compress,flask_jwt_extended,flask_login,flask_migrate,flask_sqlalchemy,flask_talisman,flask_testing,flask_wtf,freezegun,geohash,geopy,graphlib,holidays,humanize,isodate,jinja2,jwt,markdown,markupsafe,marshmallow,marshmallow_enum,msgpack,numpy,pandas,parameterized,parsedatetime,pathlib2,pgsanity,pkg_resources,polyline,prison,pyarrow,pydash,pyhive,pyparsing,pytest,pytz,redis,requests,retry,selenium,setuptools,simplejson,slack,sqlalchemy,sqlalchemy_utils,sqlparse,typing_extensions,werkzeug,wtforms,wtforms_json,yaml
multi_line_output = 3
order_by_type = false

Expand Down
1 change: 1 addition & 0 deletions setup.py
Expand Up @@ -112,6 +112,7 @@ def get_git_sha():
"wtforms-json",
"pyparsing>=2.4.7, <3.0.0",
"holidays==0.10.3", # PINNED! https://github.com/dr-prodigy/python-holidays/issues/406
"pydash>=5.0.0, <5.1.0",
"deprecation>=2.1.0, <2.2.0",
],
extras_require={
Expand Down
58 changes: 56 additions & 2 deletions superset/app.py
Expand Up @@ -14,13 +14,21 @@
# KIND, either express or implied. See the License for the
# specific language governing permissions and limitations
# under the License.
from __future__ import annotations

import importlib
import importlib.util
import logging
import os
from types import ModuleType
from typing import Any, Dict, Union

from flask import Flask
from pydash.objects import merge
from werkzeug.utils import import_string

from superset.initialization import SupersetAppInitializer
from superset.utils.core import is_test

logger = logging.getLogger(__name__)

Expand All @@ -30,8 +38,8 @@ def create_app() -> Flask:

try:
# Allow user to override our config completely
config_module = os.environ.get("SUPERSET_CONFIG", "superset.config")
app.config.from_object(config_module)
config = init_config()
app.config.from_mapping(config)

app_initializer = app.config.get("APP_INITIALIZER", SupersetAppInitializer)(app)
app_initializer.init_app()
Expand All @@ -44,5 +52,51 @@ def create_app() -> Flask:
raise ex


def init_config() -> Dict[Any, Any]:
config = convert_to_dict(load_default_config())
override_conf = convert_to_dict(load_override_config())
return merge(config, override_conf)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think bringing a new dependency (pydash) just to merge two dicts here is overkill. Why not just use dict.update?

I'm also not sure if a recursive merge is the right strategy here. If we have:

# superset/config.py
a = {"b": "c"}

And I create a custom config:

# superset_config.py
a = {"d": "e"}

This would result in the following configuration:

{'a': {'b': 'c', 'd': 'e'}}

Which is probably not what we want.

As a concrete example, we currently have languages configured in superset/config.py:

LANGUAGES = {
    "en": {"flag": "us", "name": "English"},
    "es": {"flag": "es", "name": "Spanish"},
    "it": {"flag": "it", "name": "Italian"},
    "fr": {"flag": "fr", "name": "French"},
    "zh": {"flag": "cn", "name": "Chinese"},
    "ja": {"flag": "jp", "name": "Japanese"},
    "de": {"flag": "de", "name": "German"},
    "pt": {"flag": "pt", "name": "Portuguese"},
    "pt_BR": {"flag": "br", "name": "Brazilian Portuguese"},
    "ru": {"flag": "ru", "name": "Russian"},
    "ko": {"flag": "kr", "name": "Korean"},
    "sl": {"flag": "si", "name": "Slovenian"},
}

If I want to have just Portuguese in my Superset instance I would add to my config:

LANGUAGES = {
    "pt_BR": {"flag": "br", "name": "Brazilian Portuguese"},
}

But the merge would bring back all languages.



def convert_to_dict(module: Union[ModuleType, Dict[Any, Any]]) -> Dict[Any, Any]:
raw_dict = module if isinstance(module, dict) else module.__dict__
return {k: v for k, v in raw_dict.items() if k.isupper() and not k.startswith("_")}


def load_default_config() -> ModuleType:
config_module = os.environ.get("SUPERSET_CONFIG", "superset.config")
config: ModuleType = import_string(config_module)
return config


def load_override_config() -> Union[Dict[Any, Any], ModuleType]:
CONFIG_PATH_ENV_VAR = "SUPERSET_CONFIG_PATH" # pylint: disable=C0103
if CONFIG_PATH_ENV_VAR in os.environ:
# Explicitly import config module that is not necessarily in pythonpath; useful
# for case where app is being executed via pex.
cfg_path = os.environ[CONFIG_PATH_ENV_VAR]
try:

override_conf = importlib.import_module("superset_config", cfg_path)

print(f"Loaded your LOCAL configuration at [{cfg_path}]")
return override_conf
except Exception:
logger.exception(
"Failed to import config for %s=%s", CONFIG_PATH_ENV_VAR, cfg_path
)
raise
elif importlib.util.find_spec("superset_config") and not is_test():
try:
import superset_config # pylint: disable=import-error

print(f"Loaded your LOCAL configuration at [{superset_config.__file__}]")
return superset_config
except Exception:
logger.exception("Found but failed to import local superset_config")
raise
return {}


class SupersetApp(Flask):
pass
33 changes: 2 additions & 31 deletions superset/config.py
Expand Up @@ -20,13 +20,11 @@
in your PYTHONPATH as there is a ``from superset_config import *``
at the end of this file.
"""
import imp
import importlib.util

import json
import logging
import os
import re
import sys
from collections import OrderedDict
from datetime import date
from typing import Any, Callable, Dict, List, Optional, Type, TYPE_CHECKING, Union
Expand All @@ -43,7 +41,7 @@
)
from superset.stats_logger import DummyStatsLogger
from superset.typing import CacheConfig
from superset.utils.core import is_test, parse_boolean_string
from superset.utils.core import parse_boolean_string
from superset.utils.encrypt import SQLAlchemyUtilsAdapter
from superset.utils.log import DBEventLogger
from superset.utils.logging_configurator import DefaultLoggingConfigurator
Expand Down Expand Up @@ -856,7 +854,6 @@ def CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC(
# by humans.
ROBOT_PERMISSION_ROLES = ["Public", "Gamma", "Alpha", "Admin", "sql_lab"]

CONFIG_PATH_ENV_VAR = "SUPERSET_CONFIG_PATH"

# If a callable is specified, it will be called at app startup while passing
# a reference to the Flask app. This can be used to alter the Flask app
Expand Down Expand Up @@ -1228,29 +1225,3 @@ def CSV_TO_HIVE_UPLOAD_DIRECTORY_FUNC(
# -------------------------------------------------------------------
# Don't add config values below this line since local configs won't be
# able to override them.
if CONFIG_PATH_ENV_VAR in os.environ:
# Explicitly import config module that is not necessarily in pythonpath; useful
# for case where app is being executed via pex.
try:
cfg_path = os.environ[CONFIG_PATH_ENV_VAR]
module = sys.modules[__name__]
override_conf = imp.load_source("superset_config", cfg_path)
for key in dir(override_conf):
if key.isupper():
setattr(module, key, getattr(override_conf, key))

print(f"Loaded your LOCAL configuration at [{cfg_path}]")
except Exception:
logger.exception(
"Failed to import config for %s=%s", CONFIG_PATH_ENV_VAR, cfg_path
)
raise
elif importlib.util.find_spec("superset_config") and not is_test():
try:
import superset_config # pylint: disable=import-error
from superset_config import * # type: ignore # pylint: disable=import-error,wildcard-import,unused-wildcard-import

print(f"Loaded your LOCAL configuration at [{superset_config.__file__}]")
except Exception:
logger.exception("Found but failed to import local superset_config")
raise