Skip to content

Refactor create_app in airflow/www/app.py#9291

Merged
mik-laj merged 12 commits intoapache:masterfrom
PolideaInternal:refactor-create-app
Jun 14, 2020
Merged

Refactor create_app in airflow/www/app.py#9291
mik-laj merged 12 commits intoapache:masterfrom
PolideaInternal:refactor-create-app

Conversation

@mik-laj
Copy link
Member

@mik-laj mik-laj commented Jun 14, 2020

This function was too long. It was very difficult to trace how it works. I divided it into 15 functions in 7 modules, which I am sure will facilitate its development. This file was also excluded from pylint_todo, so I fixed it to prevent regression.


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.

@mik-laj mik-laj requested a review from kaxil June 14, 2020 15:07
@boring-cyborg boring-cyborg bot added area:dev-tools area:webserver Webserver related Issues labels Jun 14, 2020
@mik-laj mik-laj requested a review from turbaszek June 14, 2020 15:14
@mik-laj mik-laj force-pushed the refactor-create-app branch from 9965f52 to 85edd82 Compare June 14, 2020 15:32

def init_xframe_protection(app):
"""
Add X-Frame-Options header. Use it avoid click-jacking attacks, by ensuring that their content is not
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Add X-Frame-Options header. Use it avoid click-jacking attacks, by ensuring that their content is not
Add X-Frame-Options header. Use it to avoid click-jacking attacks, by ensuring that their content is not

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Fixed 🐈

Comment on lines 108 to 118
from airflow.www.api.experimental import endpoints as e

# required for testing purposes otherwise the module retains
# a link to the default_auth
if app.config['TESTING']:
import importlib

importlib.reload(e)

app.register_blueprint(e.api_experimental, url_prefix='/api/experimental')
app.extensions['csrf'].exempt(e.api_experimental)
Copy link
Member

Choose a reason for hiding this comment

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

It is just used at 2 places so I think the following might be better, WDYT?

Suggested change
from airflow.www.api.experimental import endpoints as e
# required for testing purposes otherwise the module retains
# a link to the default_auth
if app.config['TESTING']:
import importlib
importlib.reload(e)
app.register_blueprint(e.api_experimental, url_prefix='/api/experimental')
app.extensions['csrf'].exempt(e.api_experimental)
from airflow.www.api.experimental import endpoints
# required for testing purposes otherwise the module retains
# a link to the default_auth
if app.config['TESTING']:
import importlib
importlib.reload(endpoints)
app.register_blueprint(e.api_experimental, url_prefix='/api/experimental')
app.extensions['csrf'].exempt(endpoints.api_experimental)

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree. We don't need an alias here.

Copy link
Member

@kaxil kaxil left a comment

Choose a reason for hiding this comment

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

2 minor comments

@mik-laj
Copy link
Member Author

mik-laj commented Jun 14, 2020

@kaxil Fixed. Thank you so much for the quick review.

@mik-laj mik-laj merged commit 2362853 into apache:master Jun 14, 2020
@mik-laj mik-laj deleted the refactor-create-app branch June 14, 2020 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools area:webserver Webserver related Issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

Comments