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

[AIRFLOW-2204] Fix webserver debug mode #3118

Merged
merged 1 commit into from
Mar 12, 2018
Merged

Conversation

bbonagura9
Copy link
Contributor

@bbonagura9 bbonagura9 commented Mar 10, 2018

JIRA

  • My PR addresses the following Airflow JIRA issues and references them in the PR title.

Description

  • Here are some details about my PR, including screenshots of any UI changes:

The command airflow webserver -d crashes because it tries to call the method run from the cached_app (from airflow.www.app) returned value, i.e. a DispatcherMiddleware instance. To run the flask app in debug mode (without gunicorn) it has to be created directly via create_app (also from airflow.www.app), that returns the Flask instance.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Very small changes, just a variable assign pushed a few lines down the usual flow and a line added for the debug flow.
(Some are to fix PEP8 compliance and pass git diff upstream/master -u -- "*.py" | flake8 --diff)

Commits

  • My commits all reference JIRA issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":

    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"
  • Passes git diff upstream/master -u -- "*.py" | flake8 --diff

@codecov-io
Copy link

codecov-io commented Mar 10, 2018

Codecov Report

Merging #3118 into master will decrease coverage by 0.02%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3118      +/-   ##
==========================================
- Coverage   73.06%   73.03%   -0.03%     
==========================================
  Files         180      180              
  Lines       12595    12596       +1     
==========================================
- Hits         9202     9200       -2     
- Misses       3393     3396       +3
Impacted Files Coverage Δ
airflow/bin/cli.py 55.02% <25%> (-0.09%) ⬇️
airflow/jobs.py 79.91% <0%> (-0.18%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76d11f2...022b7ae. Read the comment docs.

Copy link

@jgao54 jgao54 left a comment

Choose a reason for hiding this comment

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

good catch, and code lgtm! Would prefer to add a simple test to prevent regression in the future. Perhaps something like:

    def test_cli_webserver_debug(self):
        import subprocess
        p = subprocess.Popen(["airflow", "webserver", "-d"])
        sleep(3) # wait for webserver to start
        return_code = p.poll()
        self.assertEqual(None, return_code, "webserver terminated with return code {} in debug mode.".format(return_code))
        p.terminate()
        p.wait()

The command `airflow webserver -d` crashes because it tries
to call the method run from the cached_app returned value,
i.e. a DispatcherMiddleware instance. To run the flask app
in debug mode (without gunicorn) it has to be created
directly via create_app, that returns a Flask instance.

Ref: https://issues.apache.org/jira/browse/AIRFLOW-2204
@bbonagura9
Copy link
Contributor Author

@jgao54 thanks for the feedback.
I've just added the test as you suggested.

@asfgit asfgit merged commit 022b7ae into apache:master Mar 12, 2018
asfgit pushed a commit that referenced this pull request Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants