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

Allow using Airflow with Flask CLI #9030

Merged
merged 11 commits into from
Jun 2, 2020
Merged

Conversation

mik-laj
Copy link
Member

@mik-laj mik-laj commented May 27, 2020

I wanna print all routes using FLASK CLI

FLASK_APP=airflow.www.app:create_app flask routes

This will facilitate the development of Airflow.

We also have other commands in CLI

root@e7d44179d58b:/opt/airflow# FLASK_APP=airflow.www.app:create_app flask
[2020-05-27 01:19:46,966] {dagbag.py:368} INFO - Filling up the DagBag from /opt/airflow/airflow/providers/google/cloud/example_dags
/opt/airflow/airflow/utils/decorators.py:90: DeprecationWarning: Passing table parameters via keywords arguments will be deprecated. Please use provide table definition using `table_resource` parameter.You can still use external `schema_object`.
  result = func(*args, **kwargs)
Usage: flask [OPTIONS] COMMAND [ARGS]...

  A general utility script for Flask applications.

  Provides commands from Flask, extensions, and the application. Loads the
  application defined in the FLASK_APP environment variable, or from a
  wsgi.py file. Setting the FLASK_ENV environment variable to 'development'
  will enable debug mode.

    $ export FLASK_APP=hello.py
    $ export FLASK_ENV=development
    $ flask run

Options:
  --version  Show the flask version
  --help     Show this message and exit.

Commands:
  fab     FAB flask group commands
  routes  Show the routes for the app.
  run     Run a development server.
  shell   Run a shell in the app context.

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes)
  • Target Github ISSUE in description if exists
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.
Read the Pull Request Guidelines for more information.

base_url = urlparse(conf.get('webserver', 'base_url'))[2]
if not base_url or base_url == '/':
base_url = ""
flask_app.wsgi_app = DispatcherMiddleware(root_app, {base_url: flask_app.wsgi_app}) # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

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

        The actual WSGI application. This is not implemented in
        :meth:`__call__` so that middlewares can be applied without
        losing a reference to the app object. Instead of doing this::
            app = MyMiddleware(app)
        It's a better idea to do this instead::
            app.wsgi_app = MyMiddleware(app.wsgi_app)
        Then you still have the original application object around and
        can continue to call methods on it.

https://github.com/pallets/flask/blob/330a3e3ddba712def955e7a2ccee92a205dfb656/src/flask/app.py#L2323

def create_app(config=None, testing=False, app_name="Airflow"):
global app, appbuilder
Copy link
Member Author

Choose a reason for hiding this comment

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

This causes a side effect. We want the cache to be modified only by the cached_app method.

Copy link
Member

Choose a reason for hiding this comment

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

Great!

if not base_url or base_url == '/':
base_url = ""
if base_url:
flask_app.wsgi_app = DispatcherMiddleware( # type: ignore
Copy link
Member Author

Choose a reason for hiding this comment

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

This middleware is now optional. We use addresses in the form of blocked which do not work properly with this middleware. This middleware expects the addresses to be in the form /blocked. However, if we don't use this middleware, it doesn't matter.

Copy link
Member

Choose a reason for hiding this comment

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

Should we remove it then?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't use it in tests - test_views. This is still needed in production for some users.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

I do not know FAB that well but it looks good and if we can use standard flask tools to interact with the app- that's fantastic. I'd love someone else to take a look at that, but for me itis LGTM

def create_app(config=None, testing=False, app_name="Airflow"):
global app, appbuilder
Copy link
Member

Choose a reason for hiding this comment

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

Great!

if not base_url or base_url == '/':
base_url = ""
if base_url:
flask_app.wsgi_app = DispatcherMiddleware( # type: ignore
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove it then?

@mik-laj
Copy link
Member Author

mik-laj commented May 28, 2020

I still have to find one side effect because MySQL behaves differently than PostgresSQL.

@mik-laj mik-laj marked this pull request as draft May 28, 2020 17:09
@@ -29,9 +29,6 @@
from airflow.configuration import conf
from tests.test_utils.config import conf_vars

mock.patch('airflow.utils.cli.action_logging', lambda x: x).start()
Copy link
Member Author

Choose a reason for hiding this comment

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

This caused a side effect. I don't know why it affected this change. Probably some session is now saving properly.

task2 = dag2.get_task(task_id='print_the_context')
defaut_date2 = timezone.make_aware(datetime(2016, 1, 9))
dag2.clear()
Copy link
Member Author

Choose a reason for hiding this comment

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

We may have data from another run.

@@ -201,7 +202,7 @@ def test_task_states_for_dag_run(self):
tablefmt="plain")

# Check that prints, and log messages, are shown
self.assertEqual(expected.replace("\n", ""), actual_out.replace("\n", ""))
self.assertIn(expected.replace("\n", ""), actual_out.replace("\n", ""))
Copy link
Member Author

Choose a reason for hiding this comment

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

On the console, we have action_logger logs too.

@mik-laj mik-laj requested a review from turbaszek June 1, 2020 23:29
@mik-laj
Copy link
Member Author

mik-laj commented Jun 1, 2020

@turbaszek I fixed the side effects. Can you look at it again?

@mik-laj mik-laj marked this pull request as ready for review June 2, 2020 03:46
@mik-laj mik-laj merged commit 87a4a0a into apache:master Jun 2, 2020
@mik-laj mik-laj deleted the flask-cli branch June 2, 2020 10:00
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 this pull request may close these issues.

3 participants