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-5387] Fix show paused pagination bug #6100

Merged
merged 10 commits into from
Oct 8, 2019

Conversation

alrolorojas
Copy link
Contributor

@alrolorojas alrolorojas commented Sep 13, 2019

Make sure you have checked all steps below.

Jira

  • My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
    • https://issues.apache.org/jira/browse/AIRFLOW-5387
    • In case you are fixing a typo in the documentation you can prepend your commit with [AIRFLOW-XXX], code changes always need a Jira issue.
    • In case you are proposing a fundamental code change, you need to create an Airflow Improvement Proposal (AIP).
    • In case you are adding a dependency, check if the license complies with the ASF 3rd Party License Policy.

Description

This PR addresses issues with pagination + showing/hiding paused DAGs. showPaused URL parameter was removed of the URL without taking into account the value of the airflow configuration hide_paused_dags_by_default.

Now it only removes showPaused if hide_paused_dags_by_default makes it redundant.

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    • ./breeze --test-target tests.www.test_utils:TestUtils -- -s

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 (not including Jira issue reference)
    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"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Alejandro Rojas and others added 5 commits June 26, 2019 15:55
…lure

Another mistake that wasn't caught from apache#5039 - I renamed the fields
in the template (to be unique) but didn't update the view
Update Airflow Github fork from Apache Github upstream
@alrolorojas
Copy link
Contributor Author

There is a test failure that seem unrelated to the changes:

ERROR [airflow.models.dagbag.DagBag] Failed to import: /opt/airflow/tests/dags/test_impersonation_custom.py
Traceback (most recent call last):
  File "/opt/airflow/airflow/models/dagbag.py", line 199, in process_file
    m = imp.load_source(mod_name, filepath)
  File "/usr/local/lib/python3.7/imp.py", line 171, in load_source
    module = _load(spec)
  File "<frozen importlib._bootstrap>", line 696, in _load
  File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 728, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/opt/airflow/tests/dags/test_impersonation_custom.py", line 31, in <module>
    from tests.test_utils.fake_datetime import FakeDatetime
ModuleNotFoundError: No module named 'tests.test_utils'

Any idea of what could be happening?

@mik-laj
Copy link
Member

mik-laj commented Sep 14, 2019

Yesterday I prepared a fix. it is already available on masters branch. Can you do rebase onn latest apache/master?

@alrolorojas alrolorojas force-pushed the fix-show-paused-pagination-bug branch 2 times, most recently from e7474ed to 8f7bed5 Compare September 14, 2019 08:59
@alrolorojas
Copy link
Contributor Author

@mik-laj Done! Can you take a look, please?

tests/www/test_utils.py Outdated Show resolved Hide resolved
tests/www/test_utils.py Outdated Show resolved Hide resolved
@alrolorojas alrolorojas force-pushed the fix-show-paused-pagination-bug branch 2 times, most recently from 562f698 to b4d88cb Compare September 18, 2019 07:10
@alrolorojas alrolorojas force-pushed the fix-show-paused-pagination-bug branch 2 times, most recently from 7f6718e to c908cf1 Compare September 18, 2019 08:55
@alrolorojas
Copy link
Contributor Author

@feluelle @ashb I've addressed the feedback. Please take another look

Copy link
Member

@feluelle feluelle left a comment

Choose a reason for hiding this comment

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

You could even use @conf_vars to annotate the test method to use the env var only in the whole test method. But either way it is better than before (try-finally...).

LGTM 👍

@alrolorojas
Copy link
Contributor Author

alrolorojas commented Sep 18, 2019

You could even use @conf_vars to annotate the test method to use the env var only in the whole test method. But either way it is better than before (try-finally...).

@feluelle I did not find a way to pass arguments dynamically from @parameterized.expand to @conf_vars when used as a decorator. Is there a way I can do that annotating the whole method with conf_vars and use an argument from @parameterized.expand within it?

@feluelle
Copy link
Member

Good point - haven't tried @parameterized.expand along with some other annotation which needs to retrieve @parameterized.expand args.

@alrolorojas
Copy link
Contributor Author

@ashb @feluelle 👋 Is there anything else I should do as part of this PR to get it merged?

@ashb
Copy link
Member

ashb commented Oct 7, 2019

@alrolorojas Code looks good now. Could you resolve the conflicts then ping us when it's ready to merge?

@alrolorojas alrolorojas force-pushed the fix-show-paused-pagination-bug branch from b4ef638 to 09c6027 Compare October 7, 2019 17:29
@alrolorojas
Copy link
Contributor Author

@ashb @feluelle Done!

@feluelle
Copy link
Member

feluelle commented Oct 7, 2019

@alrolorojas

 import unittest
-from parameterized import parameterized
 from datetime import datetime
 from unittest import mock
 from urllib.parse import parse_qs

 from bs4 import BeautifulSoup
+from parameterized import parameterized

@alrolorojas alrolorojas force-pushed the fix-show-paused-pagination-bug branch from 09c6027 to e7ded8b Compare October 8, 2019 08:04
@alrolorojas
Copy link
Contributor Author

alrolorojas commented Oct 8, 2019

@feluelle @ashb This is completed. I do not think CI failures are related to the changes made as part of this PR.

@ashb
Copy link
Member

ashb commented Oct 8, 2019

I've re-run the failed test to make sure. If we don't merge it once it's green please ping us.

@codecov-io
Copy link

Codecov Report

Merging #6100 into master will decrease coverage by 70.84%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #6100       +/-   ##
==========================================
- Coverage   80.32%   9.47%   -70.85%     
==========================================
  Files         612     607        -5     
  Lines       35395   35035      -360     
==========================================
- Hits        28432    3321    -25111     
- Misses       6963   31714    +24751
Impacted Files Coverage Δ
airflow/www/utils.py 0% <0%> (-75.25%) ⬇️
...low/contrib/operators/wasb_delete_blob_operator.py 0% <0%> (-100%) ⬇️
airflow/example_dags/subdags/subdag.py 0% <0%> (-100%) ⬇️
airflow/gcp/sensors/bigquery_dts.py 0% <0%> (-100%) ⬇️
airflow/gcp/operators/text_to_speech.py 0% <0%> (-100%) ⬇️
airflow/contrib/sensors/emr_base_sensor.py 0% <0%> (-100%) ⬇️
airflow/gcp/hooks/discovery_api.py 0% <0%> (-100%) ⬇️
airflow/contrib/operators/gcs_list_operator.py 0% <0%> (-100%) ⬇️
airflow/example_dags/example_subdag_operator.py 0% <0%> (-100%) ⬇️
airflow/contrib/operators/file_to_gcs.py 0% <0%> (-100%) ⬇️
... and 573 more

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 0d71f33...e7ded8b. Read the comment docs.

@alrolorojas
Copy link
Contributor Author

@ashb 🍏

@feluelle
Copy link
Member

feluelle commented Oct 8, 2019

Thanks @alrolorojas

@feluelle feluelle merged commit 93bb5e4 into apache:master Oct 8, 2019
@alrolorojas alrolorojas deleted the fix-show-paused-pagination-bug branch October 8, 2019 16:01
ashb pushed a commit to ashb/airflow that referenced this pull request Oct 14, 2019
adityav pushed a commit to adityav/airflow that referenced this pull request Oct 14, 2019
@mik-laj mik-laj removed the area:UI label Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants