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-6820] split breeze into functions #7433

Merged

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Feb 16, 2020


Issue link: AIRFLOW-6820

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID*
  • Unit tests coverage for changes (not needed for documentation changes)
  • 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.

* For document-only changes commit message can start with [AIRFLOW-XXXX].


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.

@boring-cyborg boring-cyborg bot added area:dev-tools area:webserver Webserver related Issues labels Feb 16, 2020
@potiuk potiuk changed the title Airflow 6820 split breeze into functions [AIRFLOW-6820] split breeze into functions Feb 16, 2020
@potiuk
Copy link
Member Author

potiuk commented Feb 16, 2020

Hey @dimberman -> this is part of the #6500 -> I split it into smaller pieces as you suggested. First was #7430 - already approved by Ash. Now this one which introduces functions to breeze and makes it much easier to read. The next one will be to move the build logic from ./hooks/build script to _utils.sh and that will be it.

@codecov-io
Copy link

codecov-io commented Feb 16, 2020

Codecov Report

Merging #7433 into master will increase coverage by 0.25%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7433      +/-   ##
==========================================
+ Coverage   86.37%   86.62%   +0.25%     
==========================================
  Files         877      878       +1     
  Lines       41221    41292      +71     
==========================================
+ Hits        35603    35770     +167     
+ Misses       5618     5522      -96
Impacted Files Coverage Δ
airflow/configuration.py 87.78% <0%> (-0.23%) ⬇️
...viders/docker/example_dags/example_docker_swarm.py 100% <0%> (ø) ⬆️
airflow/config_templates/default_celery.py 52.94% <0%> (ø) ⬆️
...ow/providers/docker/example_dags/example_docker.py 100% <0%> (ø) ⬆️
airflow/models/connection.py 95.07% <0%> (ø) ⬆️
...low/providers/elasticsearch/hooks/elasticsearch.py 46.15% <0%> (ø)
airflow/utils/db.py 98.29% <0%> (+0.01%) ⬆️
airflow/kubernetes/refresh_config.py 74.5% <0%> (+23.52%) ⬆️
...viders/cncf/kubernetes/operators/kubernetes_pod.py 94.89% <0%> (+25.51%) ⬆️
airflow/kubernetes/pod_launcher.py 92.25% <0%> (+45.07%) ⬆️
... and 2 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 9cbd7de...c892ed2. Read the comment docs.

@potiuk potiuk removed the area:webserver Webserver related Issues label Feb 16, 2020
@potiuk potiuk force-pushed the AIRFLOW-6820-split-breeze-into-functions branch from c9dd419 to 2a5a2f8 Compare February 16, 2020 19:31
@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Feb 16, 2020
@potiuk potiuk removed the area:webserver Webserver related Issues label Feb 16, 2020
@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Feb 16, 2020
@potiuk
Copy link
Member Author

potiuk commented Feb 16, 2020

And @ashb -> that's the next step to make the breeze scripts simpler/better testable

breeze Outdated Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

I'm a little bit lost in two-in-one refactor in this PR (moving things to functions, and also renaming some env vars) but looks okay from what I can tell

breeze Show resolved Hide resolved
Copy link
Contributor

@dimberman dimberman left a comment

Choose a reason for hiding this comment

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

We should make sure this works for pyenv, but at a code level this LGTM

@potiuk
Copy link
Member Author

potiuk commented Feb 17, 2020

I'm a little bit lost in two-in-one refactor in this PR (moving things to functions, and also renaming some env vars) but looks okay from what I can tell

I could not resist the temptation to rename some of the variables and make them shorter. Since they are all internal and done automatically I think doing it while separating out functions might be a good idea.

@potiuk potiuk force-pushed the AIRFLOW-6820-split-breeze-into-functions branch from 2a5a2f8 to 1e2fbf9 Compare February 18, 2020 00:51
@potiuk
Copy link
Member Author

potiuk commented Feb 18, 2020

We should make sure this works for pyenv, but at a code level this LGTM

Confirmed it works fr both pyenv and virtualenv.

@potiuk potiuk force-pushed the AIRFLOW-6820-split-breeze-into-functions branch 3 times, most recently from 279f348 to 84fce4b Compare February 18, 2020 02:37
@potiuk potiuk requested a review from ashb February 18, 2020 20:31
@potiuk
Copy link
Member Author

potiuk commented Feb 18, 2020

Hey @ashb. Are you ok with how it is ?

hooks/build Show resolved Hide resolved
Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Looks good other than one small tidy up.

@potiuk potiuk force-pushed the AIRFLOW-6820-split-breeze-into-functions branch from 84fce4b to c892ed2 Compare February 19, 2020 00:17
@potiuk
Copy link
Member Author

potiuk commented Feb 19, 2020

Looks good other than one small tidy up.

Done

@potiuk potiuk merged commit 24f9f11 into apache:master Feb 19, 2020
potiuk added a commit that referenced this pull request Feb 19, 2020
kaxil pushed a commit that referenced this pull request Feb 21, 2020
galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020
kaxil pushed a commit that referenced this pull request Mar 19, 2020
kaxil pushed a commit to astronomer/airflow that referenced this pull request Mar 19, 2020
kaxil pushed a commit to astronomer/airflow that referenced this pull request Mar 30, 2020
(cherry picked from commit 24f9f11)
(cherry picked from commit 895c6f0)
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.

4 participants