chore(refactor): load configuration and merge recursively #15405
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ofekisr
changed the title
amitmiran137
changed the title
Codecov Report
@@ Coverage Diff @@
## master #15405 +/- ##
==========================================
- Coverage 77.23% 77.02% -0.22%
==========================================
Files 975 975
Lines 50615 50630 +15
Branches 6212 6212
==========================================
- Hits 39094 38996 -98
- Misses 11314 11427 +113
Partials 207 207
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
amitmiran137
approved these changes
Jun 28, 2021
There was a problem hiding this comment.
LGTM!
this will support a reacher config structure in #15296
8 tasks
mvoitko
pushed a commit
to mvoitko/superset
that referenced
this pull request
Jun 28, 2021
* refactor load configuration * refactor init configuration to enable recursive merge * Update config.py * Update app.py * fix lint and black issue
|
@ofekisr currently my RESULTS_BACKEND = RedisCache(host="localhost", port=6379, key_prefix="superset_results")And it failes to run because |
| 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) |
There was a problem hiding this comment.
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.
8 tasks
|
@ofekisr @amitmiran137 @betodealmeida @villebro I'm all for standardizing the configuration import logic but why don't we simply use the recommended Flask pattern , i.e., app = Flask(__name__)
app.config.from_object("superset.config")
app.config.from_envvar("SUPERSET_CONFIG_PATH", silent=True)instead of writing our own import/merge logic? |
|
@ofekisr @amitmiran137 this updated logic breaks existing config functionality, specifically where callable configuration references other configuration variables: i.e., locally overriding the Note I'm supportive of the general direction of this PR, especially given the previous logic is atypical from a Flask perspective, however the regression needs to be addressed by either i) amending the logic or ii) reverting this PR (alongside @betodealmeida's and @villebro's fixes). |
betodealmeida
added a commit
to betodealmeida/incubator-superset
that referenced
this pull request
Jul 6, 2021
8 tasks
|
@john-bodley I have PR reverting the changes: #15558 I think we need to evaluate this better, since we already tried to fix it twice and it's still breaking our deployments. |
betodealmeida
added a commit
that referenced
this pull request
Jul 6, 2021
graceguo-supercat
pushed a commit
to airbnb/superset-fork
that referenced
this pull request
Jul 6, 2021
(cherry picked from commit 83be06d)
cccs-RyanS
pushed a commit
to CybercentreCanada/superset
that referenced
this pull request
Dec 17, 2021
* refactor load configuration * refactor init configuration to enable recursive merge * Update config.py * Update app.py * fix lint and black issue
cccs-RyanS
pushed a commit
to CybercentreCanada/superset
that referenced
this pull request
Dec 17, 2021
QAlexBall
pushed a commit
to QAlexBall/superset
that referenced
this pull request
Dec 29, 2021
* refactor load configuration * refactor init configuration to enable recursive merge * Update config.py * Update app.py * fix lint and black issue
QAlexBall
pushed a commit
to QAlexBall/superset
that referenced
this pull request
Dec 29, 2021
cccs-rc
pushed a commit
to CybercentreCanada/superset
that referenced
this pull request
Mar 6, 2024
* refactor load configuration * refactor init configuration to enable recursive merge * Update config.py * Update app.py * fix lint and black issue
cccs-rc
pushed a commit
to CybercentreCanada/superset
that referenced
this pull request
Mar 6, 2024
mistercrunch
added
🏷️ bot
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION