Talisman config #7529
Talisman config #7529
Conversation
craig-rueda
force-pushed
the
talisman_config
branch
from
2c66bc3 to
a8cd011
Compare
|
@john-bodley FYI, this should be backwards compatible |
| # If you want Talisman, how do you want it configured?? | ||
| TALISMAN_CONFIG = { | ||
| 'content_security_policy': None, | ||
| 'force_https': True, |
There was a problem hiding this comment.
@craig-rueda per the Flask-Talisman documentation the force_https default is True and thus it seems we could nix this line.
There was a problem hiding this comment.
Yep, that's where I got that key. Wanted it explicit here so that others will know that it might be causing trouble (as it was for me).
There was a problem hiding this comment.
Personally I find it a little confusing to include only a subset of the defaults. Maybe there is merit in being more explicit in the comment to include i) a link to the documentation which lists the defaults, and ii) mentioning that this really isn't the entire configuration but rather an augmentation of the explicitly mentioned parameters.
There was a problem hiding this comment.
@craig-rueda would you be able to follow up on this comment in a future PR?
superset/__init__.py
Outdated
| Talisman(app, content_security_policy=None) | ||
| if app.config['TALISMAN_ENABLED']: | ||
| talisman_config = app.config.get('TALISMAN_CONFIG') or { | ||
| 'content_security_policy': None, |
There was a problem hiding this comment.
@craig-rueda any reason to have {'content_security_policy': None} as a fallback? Wouldn't one simply re-specify the options they require in their superset_config.py file if they wish to override the settings in superset/config.py?
There was a problem hiding this comment.
That was the setting that was there before - assumed it had a purpose
There was a problem hiding this comment.
This is to match the current behavior by default (Talisman(app, content_security_policy=None))
There was a problem hiding this comment.
@mistercrunch wouldn't there be merit in having the logic in superset/__init__.py be,
TALISMAN_CONFIG = {
'content_security_policy': None,
}
to match the default behavior? In this example it seems like @craig-rueda want's to have force_https_permanent to be True which should then be overridden in the superset_config.py file.
Codecov Report
@@ Coverage Diff @@
## master #7529 +/- ##
=========================================
+ Coverage 65.29% 65.3% +<.01%
=========================================
Files 432 432
Lines 21384 21388 +4
Branches 2355 2355
=========================================
+ Hits 13963 13967 +4
Misses 7301 7301
Partials 120 120
Continue to review full report at Codecov.
|
|
Ok default was removed and someone on Slack is having similar issues, merging! |
* Making Talisman configurable * Fixing double quotes * Fixing flake8 * Removing default (cherry picked from commit 21a4670)
* Making Talisman configurable * Fixing double quotes * Fixing flake8 * Removing default (cherry picked from commit 21a4670)
mistercrunch
added
🏷️ bot
* Making Talisman configurable * Fixing double quotes * Fixing flake8 * Removing default (cherry picked from commit ec5ad7a)
CATEGORY
Choose one
SUMMARY
This PR allows you to conditionally enable Talisman. There are several features that it provides which several orgs may or may not want. Related PR: #7443
REVIEWERS
@mistercrunch