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

Rewrite Extensions registry not to depend on Flask app #3466

Closed
2 tasks
arikfr opened this issue Feb 19, 2019 · 2 comments · Fixed by #3569
Closed
2 tasks

Rewrite Extensions registry not to depend on Flask app #3466

arikfr opened this issue Feb 19, 2019 · 2 comments · Fixed by #3569
Assignees
Labels

Comments

@arikfr
Copy link
Member

arikfr commented Feb 19, 2019

@jezdez
Copy link
Member

jezdez commented Feb 19, 2019

Please allow me to specify again why the backend extensions need the Flask app instance (at the moment) passed directly (and can't use flask.current_app):

  • traditional Flask extensions require manual initialization of extension instances, Flask blueprints are not enough to cover the full life cycle of Flask apps
  • our Docker based deployment workflow (contrary to e.g. a Python package based one) requires us to have a mechanism in place that extends Redash via separate dependencies (refs Allow installing additional dependencies in Docker deployment #2810) that are installed into the container
  • to be able to extend the Redash backend system with own API endpoints, we're currently requiring the use of Flask-Restful's API endpoint registry)
  • once we've moved to something else (RFC: API Documentation #2981) the app instance is still needed to since the request handler api in anything based on Flask uses the app instance to store the handlers
  • flask.current_app isn't set yet when extensions are loaded in redash.create_app since that's where the flask runtime looks for the app instance

Some ideas how to solve this:

For some examples, feel free to check out https://github.com/mozilla/redash-stmo which contains a few extensions already, including examples that:

  • extend a built-in query runner
  • extend built-in API endpoints
  • add new API endpoints
  • bundle additional React components
  • use a 3rd party Flask extension
  • additional Celery tasks

I would be extremely happy if we were not to break any of those features 😬 But I'm also happy to work on this if you'd like and make sure the extension API becomes rock solid.

@arikfr
Copy link
Member Author

arikfr commented Feb 19, 2019

Please allow me to specify again why the backend extensions need the Flask app instance (at the moment) passed directly (and can't use flask.current_app):

I should've elaborated more in the issue description... The idea here is not to prevent existing use cases, but to improve the code architecture. The registry not being coupled with Flask doesn't mean we can't use it with Flask, it just means we don't need Flask to make it work in Celery.

Currently redash.extensions.init_extensions both iterates on iter_entry_points and updates the Flask app object. Once it's done the only way to get the list of extensions is to query Flask's app. We just need to introduce some other object in the middle which we can query for extensions.

jezdez added a commit that referenced this issue Mar 11, 2019
This separates the extension registry from the Flask app and also introduces a separate registry for preriodic tasks.

Fix #3466.
@ghost ghost assigned jezdez Mar 11, 2019
@ghost ghost added the in progress label Mar 11, 2019
jezdez added a commit that referenced this issue Mar 21, 2019
This separates the extension registry from the Flask app and also introduces a separate registry for preriodic tasks.

Fix #3466.
jezdez added a commit that referenced this issue Apr 18, 2019
## What type of PR is this? (check all applicable)
<!-- Please leave only what's applicable -->

- [x] Refactor
- [x] Bug Fix

## Description

This basically makes sure that when import the redash package we don't accidentally trigger import-time side-effects such as requiring Redis.

Refs #3569 and #3466.
jezdez added a commit that referenced this issue Apr 18, 2019
This separates the extension registry from the Flask app and also introduces a separate registry for preriodic tasks.

Fix #3466.
arikfr pushed a commit that referenced this issue May 26, 2019
* Decouple extensions from Flask app.

This separates the extension registry from the Flask app and also introduces a separate registry for preriodic tasks.

Fix #3466.

* Address review feedback.

* Update redash/extensions.py

Co-Authored-By: jezdez <[email protected]>

* Minor comment in requirements.

* Refactoring after getting feedback.

* Uncoupled bin/bundle-extensions from Flas app instance.

* Load bundles in bundle script and don’t rely on Flask.

* Upgraded to importlib-metadata 0.9.

* Add missing requirement.

* Fix TypeError.

* Added requirements for bundle_extension script.

* Install bundles requirement file correctly.

* Decouple bundle loading code from Redash.

* Install bundle requirements from requirements.txt.

* Use circleci/node for build-docker-image step, too.
harveyrendell pushed a commit to pushpay/redash that referenced this issue Nov 14, 2019
…ash#3601)

## What type of PR is this? (check all applicable)
<!-- Please leave only what's applicable -->

- [x] Refactor
- [x] Bug Fix

## Description

This basically makes sure that when import the redash package we don't accidentally trigger import-time side-effects such as requiring Redis.

Refs getredash#3569 and getredash#3466.
harveyrendell pushed a commit to pushpay/redash that referenced this issue Nov 14, 2019
* Decouple extensions from Flask app.

This separates the extension registry from the Flask app and also introduces a separate registry for preriodic tasks.

Fix getredash#3466.

* Address review feedback.

* Update redash/extensions.py

Co-Authored-By: jezdez <[email protected]>

* Minor comment in requirements.

* Refactoring after getting feedback.

* Uncoupled bin/bundle-extensions from Flas app instance.

* Load bundles in bundle script and don’t rely on Flask.

* Upgraded to importlib-metadata 0.9.

* Add missing requirement.

* Fix TypeError.

* Added requirements for bundle_extension script.

* Install bundles requirement file correctly.

* Decouple bundle loading code from Redash.

* Install bundle requirements from requirements.txt.

* Use circleci/node for build-docker-image step, too.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants