Skip to content
New issue

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

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

Already on GitHub? # to your account

chore: Deprecating ENABLE_FLASK_COMPRESS #10233

Conversation

john-bodley
Copy link
Member

@john-bodley john-bodley commented Jul 2, 2020

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

This PR reverts the logic in #4617 as the Flask-Compress extension includes a COMPRESS_REGISTER option (default is True) which enables or disables compression when the extension is initialized.

TEST PLAN

CI.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI
  • Requires DB Migration.
  • Confirm DB Migration upgrade and downgrade tested.
  • Introduces new feature or API
  • Removes existing feature or API

@john-bodley john-bodley force-pushed the john-bodley--deprecate-enable-flask-request branch from f116c97 to 94ada9d Compare July 2, 2020 18:02
@john-bodley john-bodley marked this pull request as ready for review July 2, 2020 18:04
@john-bodley john-bodley requested review from villebro and etr2460 July 2, 2020 18:04
@@ -228,7 +228,7 @@ Note that the development web
server (`superset run` or `flask run`) is not intended for production use.

If not using gunicorn, you may want to disable the use of flask-compress
by setting `ENABLE_FLASK_COMPRESS = False` in your `superset_config.py`
by setting `COMPRESS_REGISTER = False` in your `superset_config.py`
Copy link
Member

@bkyryliuk bkyryliuk Jul 2, 2020

Choose a reason for hiding this comment

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

maybe add COMPRESS_REGISTER to the config.py
I tend to use the config.py as a documentation for the available configuration options.
and add the link in the comments: https://github.com/colour-science/flask-compress

Copy link
Member Author

Choose a reason for hiding this comment

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

@bkyryliuk historically config.py hasn't enumerated all the various Flask configs but rather only defines those which differ from the defaults.

Copy link
Member

@bkyryliuk bkyryliuk left a comment

Choose a reason for hiding this comment

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

LG%nit

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM

@mistercrunch
Copy link
Member

LGTM.

More generally, I'm wondering if it makes sense to avoid doing too much on the Flask app config and instructing people to use FLASK_APP_MUTATOR instead. It gets tricky to surface all the knobs and buttons that the ecosystem offers...

I'm pro having good default, but generally against having the burden of resurfacing / managing all those options...

@john-bodley john-bodley merged commit 774c23a into apache:master Jul 4, 2020
@john-bodley john-bodley deleted the john-bodley--deprecate-enable-flask-request branch July 4, 2020 19:46
auxten pushed a commit to auxten/incubator-superset that referenced this pull request Nov 20, 2020
Co-authored-by: John Bodley <john.bodley@airbnb.com>
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.37.0 labels Mar 12, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S 🚢 0.37.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants