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.

Sign up for GitHub

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
merged 1 commit into from
Nov 9, 2020
Merged

Moves provider packages scripts to dev #12082

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,10 +378,10 @@ jobs:
runs-on: ubuntu-latest
needs: [build-info, ci-images]
env:
INSTALL_AIRFLOW_VERSION: "2.0.0-dev" # Note that this causes local installation
INSTALL_AIRFLOW_VERSION: "wheel"
PYTHON_MAJOR_MINOR_VERSION: ${{needs.build-info.outputs.defaultPythonVersion}}
VERSION_SUFFIX_FOR_PYPI: "a0"
VERSION_SUFFIX_FOR_SVN: "a0"
VERSION_SUFFIX_FOR_PYPI: "a2"
ashb marked this conversation as resolved.
Show resolved Hide resolved
VERSION_SUFFIX_FOR_SVN: "a2"
if: needs.build-info.outputs.image-build == 'true'
steps:
- name: "Checkout ${{ github.ref }} ( ${{ github.sha }} )"
Expand Down
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,10 @@ dmypy.json
/.kube
/.inputrc
log.txt*

# Provider-related ignores
/provider_packages/CHANGELOG.txt
/airflow/providers/__init__.py

# Local .terraform directories
**/.terraform/*
Expand Down
15 changes: 13 additions & 2 deletions .pre-commit-config.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ repos:
hooks:
- id: isort
name: Run isort to sort imports
types: [python]
files: \.py$
ashb marked this conversation as resolved.
Show resolved Hide resolved
# To keep consistent with the global isort skip config defined in setup.cfg
exclude: ^build/.*$|^.tox/.*$|^venv/.*$
- repo: https://github.com/pycqa/pydocstyle
Expand Down Expand Up @@ -335,7 +335,7 @@ repos:
^airflow/operators/.*$|
^airflow/sensors/.*$|
^airflow/providers/.*$|
^provider_packages/.*$
^dev/provider_packages/.*$
- id: base-operator
language: pygrep
name: Make sure BaseOperator[Link] is imported from airflow.models outside of core
Expand Down Expand Up @@ -438,6 +438,17 @@ repos:
files: ^airflow/www/.*\.(css|scss|sass)$
# Keep dependency versions in sync w/ airflow/www/package.json
additional_dependencies: ['[email protected]', '[email protected]']
- id: version-sync
name: Version sync
files: ^airflow/version.py$|setup.py
entry: ./scripts/ci/pre_commit/pre_commit_sync_version.sh
Copy link
Member

Choose a reason for hiding this comment

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

What's this needed for?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was just about to write about it. The previous way of retrieving version gave already hard time in previous scenarios when setup.py was used while airflow was already installed and in pre-commits. It sometimes did not found version.py. Therefore I've added a change to synchronize version in setup.py and version.py at the pre-commit level. This way there is no importlib call during setup.py execution - version is hard-coded, so there is no risk any problems will happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

This way @ashb -> we do not have any dynamic importing while building/preparing the packages. IMHO it's much more robust and healthy if the version is hard-coded. It is especially a problem in case we want to import stuff like "EXTRAS" etc in setup.py and run those check in various "environments" - for example in pre-commits and in case of scripts that are running outside of "airflow" folder, or when we have airflow installed as pypi package rather than fro sources.

Copy link
Member

Choose a reason for hiding this comment

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

Not thrilled about this change.

When, and why, are we importing the top-level setup.py when Airflow is already installed?

Copy link
Member

Choose a reason for hiding this comment

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

(This doesn't block us merging this for now)

Copy link
Member Author

@potiuk potiuk Nov 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

There are at lest few cases:

  • We are importing it in some python pre-commits to verify setup.py consistency, ordering, completeness

  • We are importing it when automatically generating provider package setup.py's or when we generate README files for those (we are retrieving information about dependencies for each package so that we can produce a documentation showing those dependencies and add the dependencies in the generated setup.py for providers using a single source of truth.

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy if you have other proposals for those.

The provider setup.py and README generation is difficult to be done otherwise unless we built in some dump of dependencies from setup.py in some structured way, but we would need to do it as pre-commit whenever setup.py changes.

We already do that for another case - to generate cross-provider dependencies https://github.com/apache/airflow/blob/master/airflow/providers/dependencies.json - this file is a structured dump of which provider depends on which. It is done from the sources rather than from setup.py automatically using pre-commit.

But I think it would not be a good way to keep that information in two places (setup.py and another .json or similar).

Happy to discuss what other approaches we might take there if you can propose something.

Copy link
Member Author

@potiuk potiuk Nov 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

For inspiration - this is how we generate those depenencies.json . I think they are useful to keep to see the dependencies:

      - id: build-providers-dependencies
        name: Build cross-dependencies for providers packages
        entry: "./scripts/ci/pre_commit/pre_commit_build_providers_dependencies.sh"
        language: system
        files: ^airflow/providers/.*\.py$|^tests/providers/.*\.py$
        pass_filenames: false

This has been working for months now without anyone doing anything to keep those in-sync and I saw quite a few cross-provider dependencies changed between the few provider package releases we had. Thanks to that we do not have to do anything to keep provider's cross-dependencies in sync.

And we have a nice history of changes to those cross-provider deps

Screenshot from 2020-11-09 12-11-13

language: system
potiuk marked this conversation as resolved.
Show resolved Hide resolved
- id: providers-init-file
name: Provider init file
pass_filenames: false
always_run: true
entry: ./scripts/ci/pre_commit/pre_commit_check_providers_init.sh
language: system
- id: mermaid
name: Generate mermaid images
entry: ./scripts/ci/pre_commit/pre_commit_mermaid.sh
Expand Down
16 changes: 11 additions & 5 deletions BREEZE.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1207,6 +1207,9 @@ This is the current syntax for `./breeze <./breeze>`_:
image building time in production image and at container entering time for CI image. One of:

1.10.12 1.10.11 1.10.10 1.10.9 1.10.8 1.10.7 1.10.6 1.10.5 1.10.4 1.10.3 1.10.2
wheel

When 'wheel' is used, Airflow is installed from /dist/apache_airflow-*.whl file.

-t, --install-airflow-reference INSTALL_AIRFLOW_REFERENCE
If specified, installs Airflow directly from reference in GitHub. This happens at
Expand Down Expand Up @@ -2004,11 +2007,11 @@ This is the current syntax for `./breeze <./breeze>`_:
dont-use-safe-filter end-of-file-fixer fix-encoding-pragma flake8 forbid-tabs
helm-lint incorrect-use-of-LoggingMixin insert-license isort language-matters
lint-dockerfile lint-openapi mermaid mixed-line-ending mypy mypy-helm
no-relative-imports pre-commit-descriptions provide-create-sessions pydevd
pydocstyle pylint pylint-tests python-no-log-warn pyupgrade restrict-start_date
rst-backticks setup-order setup-installation shellcheck sort-in-the-wild stylelint
trailing-whitespace update-breeze-file update-extras update-local-yml-file
update-setup-cfg-file yamllint
no-relative-imports pre-commit-descriptions provide-create-sessions
providers-init-file pydevd pydocstyle pylint pylint-tests python-no-log-warn
pyupgrade restrict-start_date rst-backticks setup-order setup-installation
shellcheck sort-in-the-wild stylelint trailing-whitespace update-breeze-file
update-extras update-local-yml-file update-setup-cfg-file version-sync yamllint

You can pass extra arguments including options to to the pre-commit framework as
<EXTRA_ARGS> passed after --. For example:
Expand Down Expand Up @@ -2231,6 +2234,9 @@ This is the current syntax for `./breeze <./breeze>`_:
image building time in production image and at container entering time for CI image. One of:

1.10.12 1.10.11 1.10.10 1.10.9 1.10.8 1.10.7 1.10.6 1.10.5 1.10.4 1.10.3 1.10.2
wheel

When 'wheel' is used, Airflow is installed from /dist/apache_airflow-*.whl file.

-t, --install-airflow-reference INSTALL_AIRFLOW_REFERENCE
If specified, installs Airflow directly from reference in GitHub. This happens at
Expand Down
3 changes: 2 additions & 1 deletion CI.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,8 @@ You can use those variables when you try to reproduce the build locally.
| Image variables |
+-----------------------------------------+-------------+-------------+------------+-------------------------------------------------+
| ``INSTALL_AIRFLOW_VERSION`` | | | | Installs Airflow version from PyPI when |
| | | | | building image. |
| | | | | building image. Can be "wheel" to install from |
| | | | | the wheel package instead. |
+-----------------------------------------+-------------+-------------+------------+-------------------------------------------------+
| ``INSTALL_AIRFLOW_REFERENCE`` | | | | Installs Airflow version from GitHub |
| | | | | branch or tag. |
Expand Down
6 changes: 5 additions & 1 deletion STATIC_CODE_CHECKS.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,8 @@ require Breeze Docker images to be installed locally:
----------------------------------- ---------------------------------------------------------------- ------------
``provide-create-sessions`` Make sure provide-session and create-session imports are OK.
----------------------------------- ---------------------------------------------------------------- ------------
``providers-init-file`` Check that provider's __init__.py file is removed
----------------------------------- ---------------------------------------------------------------- ------------
``pydevd`` Check for accidentally committed pydevd statements.
----------------------------------- ---------------------------------------------------------------- ------------
``pydocstyle`` Runs pydocstyle.
Expand Down Expand Up @@ -156,10 +158,12 @@ require Breeze Docker images to be installed locally:
----------------------------------- ---------------------------------------------------------------- ------------
``update-local-yml-file`` Updates mounts in local.yml file.
----------------------------------- ---------------------------------------------------------------- ------------
``update-setup-cfg-file`` Update setup.cfg file with all licenses.
``update-setup-cfg-file`` Update setup.cfg file with all licenses.
----------------------------------- ---------------------------------------------------------------- ------------
``update-extras`` Updates extras in the documentation.
----------------------------------- ---------------------------------------------------------------- ------------
``version-sync`` Synchronizes versions setup.py <-> airflow/version.py.
----------------------------------- ---------------------------------------------------------------- ------------
``yamllint`` Checks yaml files with yamllint.
=================================== ================================================================ ============

Expand Down
6 changes: 4 additions & 2 deletions airflow/api_connexion/exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,10 @@

from airflow import version

doc_link = f'https://airflow.apache.org/docs/{version.version}/stable-rest-api-ref.html'
if 'dev' in version.version:
if any(suffix in version.version for suffix in ['dev', 'a', 'b']):
potiuk marked this conversation as resolved.
Show resolved Hide resolved
doc_link = "https://airflow.readthedocs.io/en/latest/stable-rest-api-ref.html"
else:
doc_link = f'https://airflow.apache.org/docs/{version.version}/stable-rest-api-ref.html'

EXCEPTIONS_LINK_MAP = {
400: f"{doc_link}#section/Errors/BadRequest",
Expand All @@ -38,6 +39,7 @@
def common_error_handler(exception):
"""
Used to capture connexion exceptions and add link to the type field

potiuk marked this conversation as resolved.
Show resolved Hide resolved
:type exception: Exception
"""
if isinstance(exception, ProblemException):
Expand Down
51 changes: 0 additions & 51 deletions airflow/providers/amazon/backport_provider_setup.cfg
View file
Open in desktop

This file was deleted.

112 changes: 0 additions & 112 deletions airflow/providers/amazon/backport_provider_setup.py
View file
Open in desktop

This file was deleted.

51 changes: 0 additions & 51 deletions airflow/providers/apache/cassandra/backport_provider_setup.cfg
View file
Open in desktop

This file was deleted.

Loading