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

Moves provider packages scripts to dev #12082

Merged

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.

docs/conf.py Outdated Show resolved Hide resolved
docs/conf.py Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@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.

dev/import_all_classes.py Outdated Show resolved Hide resolved
dev/import_all_classes.py Outdated Show resolved Hide resolved
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