[AIRFLOW-6089] Reorder setup.py dependencies and add ci #6681

zhongjiajie · 2019-11-27T16:02:11Z

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-6089
- 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

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

reorder setup package in alphabetical and add unittest to ci

Tests

My PR adds the following unit tests :

Add test

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

codecov-io · 2019-11-27T19:07:25Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    #6681      +/-   ##
==========================================
- Coverage   83.84%   83.41%   -0.43%     
==========================================
  Files         668      668              
  Lines       37547    37547              
==========================================
- Hits        31481    31320     -161     
- Misses       6066     6227     +161

Impacted Files	Coverage Δ
airflow/kubernetes/volume_mount.py	`44.44% <0%> (-55.56%)`	⬇️
airflow/executors/sequential_executor.py	`47.61% <0%> (-52.39%)`	⬇️
airflow/kubernetes/volume.py	`52.94% <0%> (-47.06%)`	⬇️
airflow/kubernetes/pod_launcher.py	`45.25% <0%> (-46.72%)`	⬇️
airflow/kubernetes/refresh_config.py	`50.98% <0%> (-23.53%)`	⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py	`78.2% <0%> (-20.52%)`	⬇️
airflow/utils/sqlalchemy.py	`86.44% <0%> (-6.78%)`	⬇️
airflow/executors/__init__.py	`63.26% <0%> (-4.09%)`	⬇️
airflow/configuration.py	`89.13% <0%> (-3.63%)`	⬇️
airflow/utils/dag_processing.py	`55.84% <0%> (-2.64%)`	⬇️
... and 1 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 f2f8d01...6b4561e. Read the comment docs.

zhongjiajie · 2019-11-28T01:19:43Z

CI passed in https://travis-ci.org/apache/airflow/builds/617797191?utm_source=github_status&utm_medium=notification

zhongjiajie · 2019-11-28T01:21:52Z

I'm not sure if CI work , so I add a failed commit to test if static test work.

zhongjiajie · 2019-11-28T05:57:01Z

I'm not sure if CI work , so I add a failed commit to test if static test work.

Seem Travis-CI pre-test stage don't catch the error, but test stage catch https://travis-ci.org/apache/airflow/builds/617989011?utm_source=github_status&utm_medium=notification

Have to more work on it

zhongjiajie · 2019-11-28T06:47:59Z

scripts/ci/_utils.sh

-            "--" "/opt/airflow/scripts/ci/in_container/run_check_licence.sh" \
+            "--" "/opt/airflow/scripts/ci/in_container/run_check_setup.sh" \


Found out that wrong script name we call.

zhongjiajie · 2019-11-28T07:23:13Z

Now, Travis-CI failed as we except in https://travis-ci.org/apache/airflow/builds/618058541?utm_source=github_status&utm_medium=notification
BUT NOT SHOW ERROR DETAIL

potiuk · 2019-11-29T10:41:32Z

Hey @zhongjiajie -> It's super cool change. I have one suggestion though. Your test does not require full airflow set of dependencies to run - it's rather standalone, so you can run it as a "python" pre-commit check - similar to isort. Then you would not need all the scripts (they are only needed if you want to run stuff inside docker container).

something like that could work:

      - id: setup-order
        name: Checks for an order of dependencies in setup.py
        language: python
        files: ^setup.py$
        pass_filenames: false
        require_serial: true
        entry: tests/test_order_setup.py

It will automatically create a separate virtualenv for this check and run the test in this virtualenv.

zhongjiajie · 2019-11-29T17:08:24Z

@potiuk Thanks for your advices, actually I try to understand how Airflow Breeze work during those days.

zhongjiajie · 2019-11-29T18:03:47Z

pre-commit configure work with

      - id: setup-order
        name: Checks for an order of dependencies in setup.py
        language: system
        files: ^setup.py$
        pass_filenames: false
        require_serial: true
        entry: "python ./tests/test_order_setup.py"

Will create a new commit tomorrow

potiuk · 2019-11-29T19:19:30Z

You can also run chmod +x ./tests/test_order_setup.py and add shebang "#!/usr/bin/env python" at the beginning of the file to make it executable and get it back to 'entry: tests/test_order_setup.py'

@zhongjiajie -> good that you want to learn how breeze works :). BTW. make sure to take a look at the latest updates to the docs done with Technical Writer Elena: https://github.com/PolideaInternal/airflow/blob/more-gsod-improvements/CONTRIBUTING.rst

zhongjiajie · 2019-11-30T03:11:14Z

@potiuk This PR is ready for review, PTAL

zhongjiajie · 2019-11-30T03:18:41Z

CONTRIBUTING.rst

@@ -458,6 +458,8 @@ image built locally):
 ----------------------------------- ---------------------------------------------------------------- ------------
 ``rst-backticks``                     Checks if RST files use double backticks for code.
 ----------------------------------- ---------------------------------------------------------------- ------------
+``setup-order``                       Checks for an order of dependencies in setup.py
+----------------------------------- ---------------------------------------------------------------- ------------


Not sure what Breeze field mean and not add * finally

It's explained above (and we have a change in progress that it gets explained better). It's exactly the difference whether it uses Docker Breeze image underneath or not. Your test initially used it (with the scripts etc.) but then now with "python" it does not use those images. So it's ok as it is.

zhongjiajie · 2019-11-30T03:19:34Z

BREEZE.rst

-          [ all all-but-pylint check-hooks-apply check-merge-conflict check-executables-have-shebangs check-xml debug-statements detect-private-key doctoc end-of-file-fixer flake8 forbid-tabs insert-license check-apache-license lint-dockerfile mixed-line-ending mypy pylint shellcheck].
+          [ all all-but-pylint check-apache-license check-executables-have-shebangs check-hooks-apply check-merge-conflict check-xml debug-statements doctoc detect-private-key end-of-file-fixer flake8 forbid-tabs insert-license lint-dockerfile mixed-line-ending mypy pylint setup-order shellcheck].


I make this in alphabetical, same as others hint

* Reorder dependencies in setup.py * Add static check in Airflow Breeze ci

potiuk · 2019-11-30T08:58:07Z

BREEZE.rst

-          [ all all-but-pylint check-hooks-apply check-merge-conflict check-executables-have-shebangs check-xml debug-statements detect-private-key doctoc end-of-file-fixer flake8 forbid-tabs insert-license check-apache-license lint-dockerfile mixed-line-ending mypy pylint shellcheck].
+          [ all all-but-pylint check-apache-license check-executables-have-shebangs check-hooks-apply check-merge-conflict check-xml debug-statements doctoc detect-private-key end-of-file-fixer flake8 forbid-tabs insert-license lint-dockerfile mixed-line-ending mypy pylint setup-order shellcheck].


potiuk · 2019-11-30T08:59:38Z

CONTRIBUTING.rst

@@ -458,6 +458,8 @@ image built locally):
 ----------------------------------- ---------------------------------------------------------------- ------------
 ``rst-backticks``                     Checks if RST files use double backticks for code.
 ----------------------------------- ---------------------------------------------------------------- ------------
+``setup-order``                       Checks for an order of dependencies in setup.py
+----------------------------------- ---------------------------------------------------------------- ------------


It's explained above (and we have a change in progress that it gets explained better). It's exactly the difference whether it uses Docker Breeze image underneath or not. Your test initially used it (with the scripts etc.) but then now with "python" it does not use those images. So it's ok as it is.

potiuk · 2019-11-30T09:01:03Z

Cool! very nice. Thanks @zhongjiajie !

potiuk · 2019-11-30T09:40:54Z

I also cherry-picked it to v1-10-test :)

* Reorder dependencies in setup.py (cherry picked from commit fc2098f)

zhongjiajie · 2019-11-30T17:44:20Z

Thanks Jarek's review and merge

* Reorder dependencies in setup.py (cherry picked from commit fc2098f)

* Reorder dependencies in setup.py

zhongjiajie force-pushed the order_setup_add_ci branch from 3f2ea79 to 087f8c7 Compare November 27, 2019 16:15

zhongjiajie changed the title ~~Order setup add ci~~ [AIRFLOW-6089] Order setup add ci Nov 27, 2019

zhongjiajie commented Nov 28, 2019

View reviewed changes

zhongjiajie force-pushed the order_setup_add_ci branch from 60ffc68 to 7056c3b Compare November 29, 2019 17:03

zhongjiajie force-pushed the order_setup_add_ci branch from 7056c3b to 7d104a5 Compare November 30, 2019 03:06

zhongjiajie marked this pull request as ready for review November 30, 2019 03:16

zhongjiajie changed the title ~~[AIRFLOW-6089] Order setup add ci~~ [AIRFLOW-6089] Reorder setup.py dependencies and add ci Nov 30, 2019

zhongjiajie commented Nov 30, 2019

View reviewed changes

[AIRFLOW-6089] Reorder setup.py dependencies and add ci

6b4561e

* Reorder dependencies in setup.py * Add static check in Airflow Breeze ci

zhongjiajie force-pushed the order_setup_add_ci branch from 7d104a5 to 6b4561e Compare November 30, 2019 03:25

potiuk approved these changes Nov 30, 2019

View reviewed changes

potiuk merged commit fc2098f into apache:master Nov 30, 2019

potiuk pushed a commit that referenced this pull request Nov 30, 2019

[AIRFLOW-6089] Reorder setup.py dependencies and add ci (#6681)

a648792

* Reorder dependencies in setup.py (cherry picked from commit fc2098f)

potiuk pushed a commit that referenced this pull request Nov 30, 2019

[AIRFLOW-6089] Reorder setup.py dependencies and add ci (#6681)

9caf060

* Reorder dependencies in setup.py (cherry picked from commit fc2098f)

zhongjiajie deleted the order_setup_add_ci branch November 30, 2019 17:43

eladkal pushed a commit to eladkal/airflow that referenced this pull request Dec 2, 2019

[AIRFLOW-6089] Reorder setup.py dependencies and add ci (apache#6681)

1c64ac8

* Reorder dependencies in setup.py (cherry picked from commit fc2098f)

kaxil pushed a commit that referenced this pull request Dec 12, 2019

[AIRFLOW-6089] Reorder setup.py dependencies and add ci (#6681)

e79c672

* Reorder dependencies in setup.py (cherry picked from commit fc2098f)

galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020

[AIRFLOW-6089] Reorder setup.py dependencies and add ci (apache#6681)

2b12dd2

* Reorder dependencies in setup.py

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-6089] Reorder setup.py dependencies and add ci #6681

[AIRFLOW-6089] Reorder setup.py dependencies and add ci #6681

zhongjiajie commented Nov 27, 2019 •

edited

Loading

codecov-io commented Nov 27, 2019 •

edited

Loading

zhongjiajie commented Nov 28, 2019

zhongjiajie commented Nov 28, 2019

zhongjiajie commented Nov 28, 2019

zhongjiajie Nov 28, 2019

zhongjiajie commented Nov 28, 2019

potiuk commented Nov 29, 2019

zhongjiajie commented Nov 29, 2019 •

edited

Loading

zhongjiajie commented Nov 29, 2019

potiuk commented Nov 29, 2019

zhongjiajie commented Nov 30, 2019

zhongjiajie Nov 30, 2019

potiuk Nov 30, 2019

zhongjiajie Nov 30, 2019

potiuk Nov 30, 2019

potiuk Nov 30, 2019

potiuk Nov 30, 2019

potiuk commented Nov 30, 2019

potiuk commented Nov 30, 2019

zhongjiajie commented Nov 30, 2019

		"--" "/opt/airflow/scripts/ci/in_container/run_check_licence.sh" \
		"--" "/opt/airflow/scripts/ci/in_container/run_check_setup.sh" \

		[ all all-but-pylint check-hooks-apply check-merge-conflict check-executables-have-shebangs check-xml debug-statements detect-private-key doctoc end-of-file-fixer flake8 forbid-tabs insert-license check-apache-license lint-dockerfile mixed-line-ending mypy pylint shellcheck].
		[ all all-but-pylint check-apache-license check-executables-have-shebangs check-hooks-apply check-merge-conflict check-xml debug-statements doctoc detect-private-key end-of-file-fixer flake8 forbid-tabs insert-license lint-dockerfile mixed-line-ending mypy pylint setup-order shellcheck].

[AIRFLOW-6089] Reorder setup.py dependencies and add ci #6681

[AIRFLOW-6089] Reorder setup.py dependencies and add ci #6681

Conversation

zhongjiajie commented Nov 27, 2019 • edited Loading

Jira

Description

Tests

Commits

Documentation

codecov-io commented Nov 27, 2019 • edited Loading

Codecov Report

zhongjiajie commented Nov 28, 2019

zhongjiajie commented Nov 28, 2019

zhongjiajie commented Nov 28, 2019

zhongjiajie Nov 28, 2019

Choose a reason for hiding this comment

zhongjiajie commented Nov 28, 2019

potiuk commented Nov 29, 2019

zhongjiajie commented Nov 29, 2019 • edited Loading

zhongjiajie commented Nov 29, 2019

potiuk commented Nov 29, 2019

zhongjiajie commented Nov 30, 2019

zhongjiajie Nov 30, 2019

Choose a reason for hiding this comment

potiuk Nov 30, 2019

Choose a reason for hiding this comment

zhongjiajie Nov 30, 2019

Choose a reason for hiding this comment

potiuk Nov 30, 2019

Choose a reason for hiding this comment

potiuk Nov 30, 2019

Choose a reason for hiding this comment

potiuk Nov 30, 2019

Choose a reason for hiding this comment

potiuk commented Nov 30, 2019

potiuk commented Nov 30, 2019

zhongjiajie commented Nov 30, 2019

zhongjiajie commented Nov 27, 2019 •

edited

Loading

codecov-io commented Nov 27, 2019 •

edited

Loading

zhongjiajie commented Nov 29, 2019 •

edited

Loading