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-1889] Move sensors to separate package #2875

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Dec 12, 2017

Hi all,

I'd like to move the core sensors to a separate package. Right now all the sensors are contained in
airflow/operators/sensors.py. But I'd like to split this like in /contrib/sensors/:
image

This also includes splitting the tests which are now mostly in tests/core.py which is a huge test class which is candidate for refactoring.

Make sure you have checked all steps below.

JIRA

Description

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

Tests

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

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

@Fokko Fokko force-pushed the AIRFLOW-1889-move-sensors-to-separate-package branch from 7773cc3 to 3c48741 Compare December 13, 2017 11:25
@Fokko Fokko force-pushed the AIRFLOW-1889-move-sensors-to-separate-package branch 5 times, most recently from 7c0be32 to f3b43ad Compare December 22, 2017 15:38
@Fokko Fokko force-pushed the AIRFLOW-1889-move-sensors-to-separate-package branch 6 times, most recently from 55ab8c9 to 41b692e Compare January 3, 2018 22:19
@Fokko Fokko changed the title [AIRFLOW-1889][WIP] Move sensors to separate package [AIRFLOW-1889] Move sensors to separate package Jan 3, 2018
@Fokko Fokko force-pushed the AIRFLOW-1889-move-sensors-to-separate-package branch 6 times, most recently from 82335ca to f5c03cb Compare January 4, 2018 20:56
@Fokko
Copy link
Contributor Author

Fokko commented Jan 5, 2018

@bolkedebruin what are your thoughts on this?

Please note that we have two hdfs_sensor.py, both in core and contrib :-)

@Fokko
Copy link
Contributor Author

Fokko commented Jan 5, 2018

Many changes are related to Flake compliancy, I had to shorten a lot of url's and fix the order of imports.

@Fokko Fokko force-pushed the AIRFLOW-1889-move-sensors-to-separate-package branch from f5c03cb to ce8ca2f Compare January 8, 2018 12:15
@codecov-io
Copy link

codecov-io commented Jan 8, 2018

Codecov Report

Merging #2875 into master will decrease coverage by 0.76%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2875      +/-   ##
==========================================
- Coverage   73.47%   72.71%   -0.77%     
==========================================
  Files         160      174      +14     
  Lines       12208    12764     +556     
==========================================
+ Hits         8970     9281     +311     
- Misses       3238     3483     +245
Impacted Files Coverage Δ
airflow/operators/hive_operator.py 70.73% <0%> (-2%) ⬇️
airflow/utils/log/file_processor_handler.py 89.85% <0%> (-1.45%) ⬇️
airflow/www/views.py 71.21% <0%> (-0.51%) ⬇️
airflow/operators/dagrun_operator.py 96.66% <0%> (-0.21%) ⬇️
airflow/operators/sensors.py 67.7% <0%> (-0.12%) ⬇️
airflow/utils/email.py 97.4% <0%> (ø) ⬆️
airflow/task/task_runner/base_task_runner.py 70.31% <0%> (ø) ⬆️
airflow/hooks/hive_hooks.py 40.58% <0%> (ø) ⬆️
airflow/example_dags/example_http_operator.py 100% <0%> (ø) ⬆️
airflow/sensors/sql_sensor.py 93.75% <0%> (ø)
... and 19 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 1abe7f6...11550a7. Read the comment docs.

@bolkedebruin
Copy link
Contributor

Can you rebase?

@Fokko Fokko force-pushed the AIRFLOW-1889-move-sensors-to-separate-package branch from ce8ca2f to 9becd53 Compare January 12, 2018 11:08
@Fokko
Copy link
Contributor Author

Fokko commented Jan 12, 2018

@bolkedebruin done!

Moving the sensors to seperate files increases readability of the
code. Also this reduces the code in the big core.py file.
@Fokko Fokko force-pushed the AIRFLOW-1889-move-sensors-to-separate-package branch from 9becd53 to 11550a7 Compare January 14, 2018 14:42
@Fokko
Copy link
Contributor Author

Fokko commented Jan 14, 2018

Rebased

@ashb
Copy link
Member

ashb commented Jan 15, 2018

I think sensors.py should still exist, contain a deprecation warning, but still have the sensors available in that package (until 2.0), otherwise DAGs will break with no warning, and it’s easy to maintain back compat in this case

@yati-sagade
Copy link
Contributor

LGTM, this change will make maintenance/testing of sensors much easier. I would love for this to be merged before I hack on AIRFLOW-2001.

@andyxhadji
Copy link
Contributor

Looks great, awesome work! I think we should keep sensors.py for now with a deprecation warning - I can add their removal to the scope of AIRFLOW-1922 for 2.0.

@asfgit asfgit closed this in 33c7204 Jan 19, 2018
@Acehaidrey
Copy link
Contributor

I'm really glad you guys did this!

Acehaidrey pushed a commit to Acehaidrey/incubator-airflow that referenced this pull request Jan 19, 2018
Moving the sensors to seperate files increases
readability of the
code. Also this reduces the code in the big
core.py file.

Closes apache#2875 from Fokko/AIRFLOW-1889-move-sensors-
to-separate-package
@Fokko
Copy link
Contributor Author

Fokko commented Jan 21, 2018

Follow up fix: #2961

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

7 participants