Skip to content

Conversation

@potiuk
Copy link
Member

@potiuk potiuk commented Nov 4, 2020

The change #10806 made airflow works with implicit packages
when "airflow" got imported. This is a good change, however
it has some unforeseen consequences. The 'provider_packages'
script copy all the providers code for backports in order
to refactor them to the empty "airflow" directory in
provider_packages folder. The #10806 change turned that
empty folder in 'airflow' package because it was in the
same directory as the provider_packages scripts.

Moving the scripts to dev solves this problem.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
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.

@potiuk potiuk force-pushed the move-provider-package-scripts-to-dev branch from 3b6da4f to 2b9f40c Compare November 4, 2020 10:21
@potiuk potiuk requested review from ashb, kaxil and mik-laj November 4, 2020 10:22
@potiuk
Copy link
Member Author

potiuk commented Nov 4, 2020

Hey @ashb -> the fix to multiple dirs broke automated import check for package provider readme preparation. I fixed it in this PR by moving the scripts to dev and adding few updates.

ashb
ashb previously approved these changes Nov 4, 2020
@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Nov 4, 2020
@github-actions
Copy link

github-actions bot commented Nov 4, 2020

The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run it!

@github-actions
Copy link

github-actions bot commented Nov 4, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

github-actions bot commented Nov 4, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@potiuk potiuk force-pushed the move-provider-package-scripts-to-dev branch from 2b9f40c to fbcb9f1 Compare November 4, 2020 12:08
@github-actions
Copy link

github-actions bot commented Nov 4, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@potiuk potiuk force-pushed the move-provider-package-scripts-to-dev branch 2 times, most recently from a17d864 to fe52890 Compare November 4, 2020 17:22
@potiuk potiuk requested a review from ashb November 4, 2020 17:42
@potiuk
Copy link
Member Author

potiuk commented Nov 4, 2020

This should hopefully work now. I've added missing setup.py/setup.cfg from the last readme preparation for providers (they have not been committed) and regenerated all the backport providers setup.py's to include "find_namespace_packages" changes. More changes than before so I re-requested review.

@github-actions
Copy link

github-actions bot commented Nov 4, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@potiuk potiuk force-pushed the move-provider-package-scripts-to-dev branch from fe52890 to 1cbfa93 Compare November 4, 2020 18:09
@github-actions
Copy link

github-actions bot commented Nov 4, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@potiuk potiuk force-pushed the move-provider-package-scripts-to-dev branch 2 times, most recently from ed6c81a to 2850cfd Compare November 4, 2020 20:33
@potiuk
Copy link
Member Author

potiuk commented Nov 4, 2020

Hey @ashb - you might want to take a second look. I actually decided that storing of those setup.py is a terrible idea and I got rid of those. The only remaining ones are READMES/CHANGES but the setup.py are generated dynamically during package generation.

@potiuk
Copy link
Member Author

potiuk commented Nov 4, 2020

Screenshot from 2020-11-04 21-36-26

@github-actions
Copy link

github-actions bot commented Nov 4, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@ashb
Copy link
Member

ashb commented Nov 4, 2020

The only remaining ones are READMES/CHANGES but the setup.py are generated dynamically during package generation.

I honestly thought that was what we were doing anyway :D Yay for deleting code.

@ashb
Copy link
Member

ashb commented Nov 5, 2020

Moving to dev/ is a better plan anyway, but this diff:

https://github.com/apache/airflow/pull/10806/files#diff-621ffdeb5947325a83db39df2bda320f901a362ff81b5657c301184aeed78535R81-R84


# We need to make sure we are not in the airflow checkout, otherwise it will automatically be added to the
# import path
cd /
python3 /import_all_provider_classes.py

was how I handled this before -- that cd won't be needed anymore either I don't think.

@ashb
Copy link
Member

ashb commented Nov 6, 2020

Nothing serious left from me -- I will likely be away from computer for most of the weekend now

@potiuk potiuk force-pushed the move-provider-package-scripts-to-dev branch 2 times, most recently from 0f89a19 to 46d83b3 Compare November 6, 2020 17:46
@potiuk
Copy link
Member Author

potiuk commented Nov 6, 2020

All fixed

@potiuk potiuk added the priority:critical Showstopper bug that should be patched immediately label Nov 7, 2020
@potiuk potiuk force-pushed the move-provider-package-scripts-to-dev branch from 46d83b3 to 7246cfe Compare November 8, 2020 15:31
@potiuk
Copy link
Member Author

potiuk commented Nov 8, 2020

Rebased after @mik-laj fix to readthedocs #12161 fix fixing implicit providers namespaces for ReadTheDocs.

The change apache#10806 made airflow works with implicit packages
when "airflow" got imported. This is a good change, however
it has some unforeseen consequences. The 'provider_packages'
script copy all the providers code for backports in order
to refactor them to the empty "airflow" directory in
provider_packages folder. The apache#10806 change turned that
empty folder in 'airflow' package because it was in the
same directory as the provider_packages scripts.

Moving the scripts to dev solves this problem.
@potiuk potiuk force-pushed the move-provider-package-scripts-to-dev branch from 7246cfe to 40a587d Compare November 9, 2020 10:55
@potiuk
Copy link
Member Author

potiuk commented Nov 9, 2020

All fixed .

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'll take a look at fixing my minor complaints in a future PR.

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Nov 9, 2020
@github-actions
Copy link

github-actions bot commented Nov 9, 2020

The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run it!

@potiuk potiuk merged commit b2a28d1 into apache:master Nov 9, 2020
@potiuk potiuk deleted the move-provider-package-scripts-to-dev branch November 9, 2020 12:27
ashb added a commit to astronomer/airflow that referenced this pull request Nov 10, 2020
A bad rebase in apache#12082 deleted this file by mistake.

This missing file was also the cause of needing the documentation
to exclude these files

Fixes apache#12239
kaxil pushed a commit that referenced this pull request Nov 10, 2020
A bad rebase in #12082 deleted this file by mistake.

This missing file was also the cause of needing the documentation
to exclude these files

Fixes #12239
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:dev-tools full tests needed We need to run full set of tests for this PR to merge priority:critical Showstopper bug that should be patched immediately

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants